Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make AssetsPath available in external_template context #46

Closed
moorereason opened this issue Apr 3, 2024 · 3 comments · Fixed by #47
Closed

Make AssetsPath available in external_template context #46

moorereason opened this issue Apr 3, 2024 · 3 comments · Fixed by #47
Labels
enhancement New feature or request

Comments

@moorereason
Copy link
Contributor

moorereason commented Apr 3, 2024

When building a custom template using the external_template format, the .AssetsPath value is not available to the template engine context.

@moorereason moorereason changed the title Make AssetsPath available in external_template context Make AssetsPath and Report available in external_template context Apr 3, 2024
@moorereason moorereason changed the title Make AssetsPath and Report available in external_template context Make AssetsPath available in external_template context Apr 3, 2024
@tierpod
Copy link
Owner

tierpod commented Apr 4, 2024

Hello, thank you for the report.

Could you please explain this use case more detailed? IIRC, I haven't exported this variable for external template on purpose. I assume, if you use external_template (and you write this template) you can hardcode necessary assets path in this template.

@moorereason
Copy link
Contributor Author

First, I've been looking for a simple tool like dmarc-report-converter that doesn't require a bunch of resources and infrastructure (preferably written in Go). I've setup an AWS SES sub-domain to deliver reports to an S3 bucket, which I pull down before running dmarc-report-converter. It's working great so far, so thank you for all of the hard work you've put into this project before I showed up complaining about things. 😆

My use case started with wanting to hack on the default template (htmlTmpl from consts.go). I copied the contents to a separate file and ran it. Right out of the gate, I got errors and had to define my own template variable and rewrite all references of .AssetsPath to $AssetsPath. This is when I opened this issue (in haste!).

But then I quickly realized that the default contexts are completely different. The external template's default context is just the pkg/dmarc.Report struct. So, I had to rewrite all of that, too (something like s/({{\s*\.)Report./$1/g).

I find all of that an annoying learning curve and a barrier to entry for newcomers (like me). I don't see the harm in exporting AssetsPath. If the user doesn't want to use it, they don't have to. IMHO, it would be more intuitive to have the same contexts in each template so that I could use the built-in templates as a guide.

Potential Fix

A potential backward-compatible fix could be something like:

type TmplCtx struct {
	AssetsPath      string
	Report          dmarc.Report

	// Deprecate these properties
	XMLName         xml.Name
	ReportMetadata  dmarc.ReportMetadata
	PolicyPublished dmarc.PolicyPublished
	Records         []dmarc.Record
	MessagesStats   dmarc.MessagesStats
}

@tierpod
Copy link
Owner

tierpod commented Apr 5, 2024

Thank you for explanation.

I understand your use case and it's completely valid. I agree that it would be better for new users to have consistent variables across all kind of templates (didn't think about it when I implemented external template).

@tierpod tierpod added the enhancement New feature or request label Apr 5, 2024
moorereason added a commit to moorereason/dmarc-report-converter that referenced this issue Apr 6, 2024
Update the template context for the external_template so that it has the
same context as the built-in templates.

Fixes tierpod#46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants