-
Notifications
You must be signed in to change notification settings - Fork 587
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
Automatically reload settings profile if it's redefined #4153
Conversation
f1e700f
to
4e69028
Compare
4e69028
to
ef002f9
Compare
I'm overall -1 on this change, because it adds yet another complication to the already complicated behavior of our settings, and it's easy enough to get the same effect by typing I'd even like to warn users about profile name collisions someday, and explicitly encouraging overrides feels counterproductive for that. |
Hmm I think it actually decreases how complicated the system is by removing things it's possible to get wrong. It means that there's a coherent concept of "the current profile" which you can never be out of sync with. I think if we didn't want profiles to be redefinable (which I'm not convinced of as a goal) we shouldn't have added a "ci" profile, and we can't add any other profiles in future. |
Right, thinking about it as " With that in mind I'd move most of the new docs to the docstring of |
if is_in_ci(): # pragma: no cover | ||
if is_in_ci(): # pragma: no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Also apparently unneeded given that coverage tests aren't failing!
This is me following up on my note for later.
Previously if you reregistered a profile you'd need to then load it again before it came into use. This changes the logic so that's not the case.
The main motivation for this is is that if a user registers a CI profile this now ensure it will automatically be used on CI.