-
-
Notifications
You must be signed in to change notification settings - Fork 258
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
Fix handling of pre-release option. #424
Conversation
Can someone please re-run the jobs.
Since all other language versions are passing I think that re-run or fix of Travis configuration may enable this change to pass the tests completely. Before submitting this PR, I tested with |
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.
lgtm. thanks for the PR.
pex/bin/pex.py
Outdated
@@ -565,7 +565,8 @@ def build_pex(args, options, resolver_option_builder): | |||
interpreters=interpreters, | |||
platforms=options.platform, | |||
cache=options.cache_dir, | |||
cache_ttl=options.cache_ttl) | |||
cache_ttl=options.cache_ttl, | |||
allow_prereleases=resolver_option_builder._allow_prereleases) |
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.
actually, one quick ask: this access of _allow_prereleases
violates the private convention. would you mind exposing _allow_prereleases
here as a public attribute of the ResolverOptionsBuilder
class and using that instead? a quick @property def allow_prereleases(self): return self._allow_prereleases
would work.
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.
Sure. That's easy. The only reason I did not do it initially is because I did not know what would be the code owner's preferences. I'll do it and resubmit.
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.
Actually, I just realized that there was another reason. The existing method:
def allow_prereleases(self, allowed):
self._allow_prereleases = allowed
return self
is not exactly a property setter, because it returns self
.
I'll try to work around the existing interface and see what feels the best.
e51022f
to
6b69415
Compare
Pex takes a `--pre` option from the command line, but it does not pass it correctly during build. We need to propagate it to resolver to get the correct behavior. Fixes pex-tool#423
6b69415
to
cbb9e57
Compare
@kwlzn I added the change and the comment describing why the property has a different name/API to-do note. If you feel I should take any of the new comments out of the patch, please let me know. |
@zvezdan thanks - merged. |
Pex takes a
--pre
option from the command line, but it does not passit correctly during build. We need to propagate it to resolver to get
the correct behavior.
Fixes #423