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

Introduce support of add and del operations under erl_opts directive #1798

Merged
merged 1 commit into from
Jun 7, 2018

Conversation

juise
Copy link
Contributor

@juise juise commented May 31, 2018

No description provided.

@ferd
Copy link
Collaborator

ferd commented May 31, 2018

Can you provide the rationale for this change so that we can review it with the proper context?

@juise
Copy link
Contributor Author

juise commented May 31, 2018

Yeah, sure. We have rebar.config, with directive erl_opts which contains opts like 'warn_missing_spec', which is necessary and helpful on compile, but is very annoying when tests are run. Just override erl_opts in test profile is not correst, in erl_opts may be useful opts like pars_transform. So, we need the way to remove some options from erl_opts directive.

Additionally I've do this for deps too, this will allow to keep 'override' option consistancy.

@fenollp
Copy link
Contributor

fenollp commented Jun 1, 2018

Did you try adding nowarn_missing_spec to the test profile?

@juise
Copy link
Contributor Author

juise commented Jun 1, 2018

@fenollp yeah, this doesn't work when you have dependencies, with warn_missing_spec flag in opts. But it's works for single app w/o deps.

@ferd
Copy link
Collaborator

ferd commented Jun 6, 2018

After looking at it, one tricky aspect of it is that I think this will die when trying to delete an option that is not a list. For example, {minimum_otp_vsn, "17.4"} cannot be deleted in any way with this option, only overridden. This option can't be added either since add only works for lists so I guess it's fine.

Thanks for providing tests as well!

I'll be discussing this a bit with Tristan with regards to its inclusion.

@ferd
Copy link
Collaborator

ferd commented Jun 7, 2018

Alright. Under the light of recent issues with things like warn_as_errors and other irreversible options, we consider this to be a good idea to introduce to rebar3 as a feature :)

Thanks for the contribution!

@ferd ferd merged commit e2e7f65 into erlang:master Jun 7, 2018
@juise
Copy link
Contributor Author

juise commented Jun 7, 2018

Awesome, thanks

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