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

Prefer setting preferences only within the currently active project #37

Merged
merged 2 commits into from
Apr 20, 2022

Conversation

staticfloat
Copy link
Member

Previous to this change, we would search up the environment stack
looking for the project that contains the target package, and set the
preference in there. This was intended to make it convenient to e.g.
set a Revise preference while having an application project activated.
It turns out that the more common problem is actually that a package
wants to set a preference of a sub-package (e.g. MPI and
MPIPreferences) that may not be a top-level dependent of the currently
active project.

Therefore, we change the behavior of set_preferences!() to prefer
adding the target package as an extras dependency of the
currently-active project, but maintain the old behavior behind a keyword
argument to set_preferences!().

Previous to this change, we would search up the environment stack
looking for the project that contains the target package, and set the
preference in there.  This was intended to make it convenient to e.g.
set a `Revise` preference while having an application project activated.
It turns out that the more common problem is actually that a package
wants to set a preference of a sub-package (e.g. `MPI` and
`MPIPreferences`) that may not be a top-level dependent of the currently
active project.

Therefore, we change the behavior of `set_preferences!()` to prefer
adding the target package as an `extras` dependency of the
currently-active project, but maintain the old behavior behind a keyword
argument to `set_preferences!()`.
@codecov
Copy link

codecov bot commented Apr 19, 2022

Codecov Report

Merging #37 (cdca172) into master (80b5509) will increase coverage by 9.19%.
The diff coverage is 84.21%.

@@            Coverage Diff             @@
##           master      #37      +/-   ##
==========================================
+ Coverage   76.52%   85.71%   +9.19%     
==========================================
  Files           2        2              
  Lines         115      126      +11     
==========================================
+ Hits           88      108      +20     
+ Misses         27       18       -9     
Impacted Files Coverage Δ
src/Preferences.jl 85.43% <84.21%> (+10.43%) ⬆️
src/utils.jl 86.95% <0.00%> (+4.34%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@DilumAluthge
Copy link
Member

Technically breaking?

@@ -1,7 +1,7 @@
name = "Preferences"
uuid = "21216c6a-2e73-6563-6e65-726566657250"
authors = ["Elliot Saba <elliot.saba@juliacomputing.com>", "contributors"]
version = "1.2.5"
version = "1.3.0"
Copy link
Member

@DilumAluthge DilumAluthge Apr 19, 2022

Choose a reason for hiding this comment

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

Suggested change
version = "1.3.0"
version = "2.0.0-DEV"

Unless you know you don't plan on making any other breaking changes anytime soon, in which case you could just set it to 2.0.0 and cut a new release after this PR is merged.

@staticfloat
Copy link
Member Author

I'm not really sure this counts as breaking.... I don't think anyone is relying on this implementation detail, and it wasn't even documented. @vchuravy what do you think?

@DilumAluthge
Copy link
Member

Hmmm, if the original behavior wasn't documented, then it's probably not breaking to change the behavior.

@vchuravy
Copy link
Member

One could even argue that it's a bug fix since we have in the docs:

The above snippet first clears the "compiler_options" key of any inheriting influence,
then sets a preference option, which guarantees that future loading of that preference
will be exactly what was saved here. If we wanted to re-enable inheritance from higher
up in the chain, we could do the same but passing missing first.

Which didn't work. We would wipe the preferences higher up in the chain instead.

@staticfloat staticfloat merged commit b5bc284 into master Apr 20, 2022
@staticfloat staticfloat deleted the sf/prefer_active_project branch April 20, 2022 06:52
@giordano giordano linked an issue Apr 20, 2022 that may be closed by this pull request
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.

Target of set_preferences unclear
3 participants