-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Unstable Feature Flags #5727
Unstable Feature Flags #5727
Conversation
Should this get a |
LOL. This unintentionally includes #5726. I'll rebase on master later today, if that PR isn't merged by then. |
Probably needs something added to the docs (in the developer section) as I've no idea what this would mean or how I'd use it, as it stands. I don't expect to use it for PEP 517, in any case. |
Yeah, I was adding that -- specifically in the deprecation policy portion. There's no good place to put it in for explaining how to use them though. |
9bd7fb4
to
f607ccd
Compare
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.
A few comments.
You could still have an independent easily tested helper function that is
called by the Option subclass. And parse_args() is easy to test on top of
that to make sure the subclass is wired up and operating correctly.
…On Fri, Aug 24, 2018 at 7:10 AM Pradyun Gedam ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/pip/_internal/utils/unstable.py
<#5727 (comment)>:
> +from pip._internal.exceptions import UnknownUnstableFeatures
+
+
+class UnstableFeaturesHelper(object):
+ """Handles logic for registering/validating/checking
+ """
+
+ def __init__(self):
+ super(UnstableFeaturesHelper, self).__init__()
+ self._names = set()
+ self._enabled_names = set()
+
+ def register(self, *names):
+ self._names.update(set(names))
+
+ def validate(self, given_names):
I don't want to dwell too deep into optparse tbh. I'd actually looked
into that specific example but the fact that we'd need to use a custom
Option subclass felt a little unnecessary. Further, keeping this separate
makes it easier to test and it's overall a simpler change.
it doesn't look like you're testing that validate() is actually called
inside main().
Indeed. I'll look into adding a test for this. :)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#5727 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAVt7pcXUENcfsqMujD0IsN3R1kIH5Gwks5uUAlUgaJpZM4WJB4w>
.
|
afadf75
to
2ffe967
Compare
I ended up with defining a new subclass that's being used in just one specific option. It'd be doing something that's equally well handled by a single line in an existing function and is tested using the exact same testing patterns (call a method and check some monkey-patched mock was called). Further, patching the helper is slightly more tedious in the case of using an Option subclass. I feel it's better to just have the helper be called in |
I still don't understand the point of this change. If I'm implementing a feature, how would I make it conditional on an "unstable feature flag" and why would doing so be any better than simply adding an IMO, this sounds like we're adding a policy about how we add new features, but I don't really understand what that policy is, or why we need it. You say
but I don't understand this. To me, it's simple - if you want to introduce an incomplete function, either simply don't document it (hence there's no backward compatibility guarantee, as there's no supported behaviour) or document it as "subject to change". If you're breaking existing behaviour, you should wait until you have all the pieces in place before doing so. PEP 517 is a perfect case in point. I'm ensuring that whatever I release passes the full test suite before I merge it, and I'm holding off on documenting the new features or adding tests for new functionality until I have a solid backward compatible foundation (that's not always easy - ahem setuptools dependency 😄 - but it's possible, and IMO worthwhile). |
This should be put in the documentation somewhere...
IMO, this provides a clearer sense of "don't consider this stable/complete" than a
IMO big feature PRs aren't usually very easy to work with. They have more changes in a single go, almost always are harder to review and hold up work in the code being modified (or need to constantly keep syncing up). IIRC during the discussion, the motivating case for this was PEP 518 -- we had a potentially "broken" implementation and we had to hold off the release till we fixed the issues on that front. Then, communicate effectively that the functionality wasn't complete. With date based releases, we can't really hold off for very long so having a mechanism to clearly mark functionality as "in-development" while still being able to iterate would be helpful. Additionally, this would serve as a much easier path to get new functionality in the hands of early adopters to get feedback, before switching the default for everyone.
Not really. The change here is that for changes where we might want some additional testing "in the wild" or feedback from users about some new functionality/behavior, we now have an(other) established way to do so. |
OK. I agree that PEP 518 was a problem because it blocked release for a long time. But IMO that was at least in part because of how it was managed. I don't want to rake over old discussions, but I do believe that it could have been handled better. I'm hoping that PEP 517 will be evidence that this is possible. Note that pretty much all of the preparatory work for PEP 517 has been merged into master and could be released at any time with no user-visible impact, and what remains is (hopefully!!!) relatively small changes to actually call the backend hooks. Anyway, I'm not going to block this change just because I don't see the need for it. If people want it, that's fine - but I don't expect to use it myself. |
Same and agreed. |
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
I'm gonna go ahead and close this since I don't have the time currently to work through all the social/technical implications of adding this. |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This has come up again in the dependency resolution work and is being discussed on Zulip: https://python.zulipchat.com/#narrow/stream/218659-pip-development/topic/opt-in.20flag |
I think we do need to talk about the possibility of doing this. @pradyunsg could I ask you to reopen? |
@brainwane I think I deleted the corresponding branch, but it can be revived pretty easily. We did end up implementing a diluted variant of this stuff in a PR by @uranusjr though. |
#7845 is a simplified version of this. I thought about how we should eventually switch in the resolver yesterday, and it occured to me
With the current flag setup, phase 1 is covered by Even step 3 could be a problem. Say we go with the above route (setting |
From my point of view, phase 1) can be enabled with |
Maybe we could have a
This has the advantage (for me) of very clearly saying that people opting out of the new behaviour in (2) can't do so indefinitely, so we have a clear path to removing the old code. |
Note that #8371 covers this. |
This PR adds a helper and a general CLI option to provide functionality for feature flags. This was something @dstufft suggested in the same discussion where we decided to switch to CalVer.
Rationale
With the new release cycle and deprecation policy, it would be difficult to incrementally introduce new functionality while maintaining those guarantees. This is particularly true for big-ticket items like #988, #4575, #4659 and possibly PEP 517 too, which will make fairly extensive user-visible changes.
Feature Flags serve as a tool to enable incrementally implementing new functionality while not having to ensure backward compatibility of it with previous iterations of itself. Any user-visible behavior hidden behind a feature flag will not be subject to backward compatibility guarantees (and neither will the names of the feature flags).
Additional benefits include: