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

Use a Dune dialect to preprocess eml templates #2781

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

aantron
Copy link
Contributor

@aantron aantron commented Oct 17, 2024

This spiritually resurrects #1181 by @gpetiot which stalled, as I understand it, due to lack of a Dream release that would make dialect support ergonomic. Because of the Dream release, the present PR doesn't need a wrapper around dream_eml.

It should also make #2764 unnecessary.

@cuihtlauac
Copy link
Collaborator

Thanks @aantron

In #1181, @emillon made this comment:

Sidenote, I had a look at the diff in this PR. If I understand correctly you're replacing N rules with each 1 dependency and 1 target, by a single rule that has N dependencies and N targets.

What's the status now? Are all the .eml going to be processed if a single one changes?

@aantron
Copy link
Contributor Author

aantron commented Oct 18, 2024

AFAIK that comment referred to an approach that was being used in that PR (12c9134#diff-85098e336ea5f1f8dca91cda539ff5f46db068c37b959eb70a32d02bc591941fL6) that had all the template outputs depend on all the template inputs, before @gpetiot switched to using a Dune dialect in the commit from that PR that I am linking in this comment. I also had that concern for a moment before I realized that, but AFAIK there is no reason to suspect that Dune dialects have this downside.

Copy link
Collaborator

@sabine sabine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks @aantron!

@sabine sabine merged commit 719c3a3 into ocaml:main Oct 25, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants