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

feat(dune): support + prefixes for PPX CLI flags #11234

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

anmonteiro
Copy link
Collaborator

@anmonteiro anmonteiro commented Dec 20, 2024

this is useful for cases like melange.ppx which emit alerts and allow to configure them via -alert ....

@anmonteiro anmonteiro changed the title feat(dune): support + prefixes for PPX CLI flags feat(dune): support + prefixes for PPX CLI flags Dec 20, 2024
@rgrinberg
Copy link
Member

I think this needs to be versioned. You can version this under dune's own language or melange's. Depending on how stable you'd like to keep this feature.

@anmonteiro
Copy link
Collaborator Author

Why do you think this needs to be versioned?

I believe the previous behavior would just plainly return an error, so we shouldn't have anyone depending on these features, right?

@rgrinberg
Copy link
Member

rgrinberg commented Dec 21, 2024

Suppose that somebody would try to use this feature in a publicly released project. That person would need to remember to update the constraints in their project file to require at least dune 3.18. Without the version check, it is very likely that the person would forget to add this constraint, or they might even do it incorrectly, if they misremember which version of dune introduced this feature. Downstream user of this project might then encounter build failures should they build with an older version of dune. If we add a version check, we make this scenario more or less impossible.

It's true that the above argument is applicable to basically any user visible change in dune, but I think it works best for features. As for bug fixes, it's good that users can benefit from those without opting in. I think that this PR is certainly more of a feature than a bug fix.

@anmonteiro
Copy link
Collaborator Author

absolutely, thanks for the explainer!

@anmonteiro anmonteiro force-pushed the anmonteiro/melange-test-use-another-flag branch 2 times, most recently from 8cdabdc to 493b6f9 Compare December 21, 2024 05:54
@anmonteiro
Copy link
Collaborator Author

@rgrinberg ready for another look.

with
| Yes, _ -> Right s
| _, Yes when syntax_version >= (3, 18) -> Right s
| (No | Unknown _), _ ->
Copy link
Member

Choose a reason for hiding this comment

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

I think you could improve the error message here with a hint for prefix = "+" when syntax_version < (3, 18)

@anmonteiro anmonteiro force-pushed the anmonteiro/melange-test-use-another-flag branch from 493b6f9 to e57cc15 Compare December 23, 2024 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants