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: introduce env_vars alias for env-vars #11442

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

maiste
Copy link
Collaborator

@maiste maiste commented Feb 4, 2025

This PR introduces an alias env_vars, for env-vars, to keep the configuration uniformity (using the underscore instead of dash).

Closes #11424

@maiste maiste requested a review from rgrinberg February 4, 2025 11:26
Copy link
Collaborator

@nojb nojb left a comment

Choose a reason for hiding this comment

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

I don't think it is a good idea to allow both (env-vars) and (env_vars) (having more than one name for the same thing is generally bad). So (env-vars) should be renamed to (env_vars) in 3.18 (see Dune_lang.Syntax.renamed_in). That way, someone who upgrades to Dune lang 3.18 is forced to rename env-vars to env_vars.

@maiste maiste added the config-file Everything related to dune configuration file (workspace, project, dune) label Feb 4, 2025
@maiste
Copy link
Collaborator Author

maiste commented Feb 5, 2025

I agree! But is it a good idea to introduce it as a breaking change for 3.18 and not for 4.0?

@maiste
Copy link
Collaborator Author

maiste commented Feb 6, 2025

@nojb what do you think about allowing both but emit a warning for now with env-vars, and removing it for 4.0.0?

@rgrinberg
Copy link
Member

I don't particularly care if we have more than one name, but I'm strongly against making breaking changes that offer no benefits to the user. So even if we introduce a new name, we need to keep supporting the old name even in 4.0. We need much more convincing reasons to break user builds.

@nojb
Copy link
Collaborator

nojb commented Feb 6, 2025

I don't particularly care if we have more than one name, but I'm strongly against making breaking changes that offer no benefits to the user. So even if we introduce a new name, we need to keep supporting the old name even in 4.0. We need much more convincing reasons to break user builds.

OK!

@maiste
Copy link
Collaborator Author

maiste commented Feb 6, 2025

OK, so let's stick to the two names, then.

@maiste maiste force-pushed the fix/11424-consistency branch from dde98f9 to 763a10c Compare February 6, 2025 12:20
@maiste maiste force-pushed the fix/11424-consistency branch from 763a10c to 7d541f7 Compare February 11, 2025 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config-file Everything related to dune configuration file (workspace, project, dune)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

consistency in dashes Vs underscores (e.g. "env-vars")
3 participants