Improve accuracy of _alwayscopy_not_supported() #1133
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gaborbernat
requested changes
Jan 10, 2019
There was a problem hiding this comment.
@jdufresne can you please the linting? just run tox -e fix_lint, thanks!
Also, we have a test dependency inside setup.py on distro if you remove it from where you can remove from there too 👍
Furthermore, I feel if you make this check like this we should probably cache it in a global variable, to reuse it for subsequent calls 👍
Previously, this function checked against a whitelist of OSes that were known to be a problem. This didn't include Fedora, which also fails this test. Rather than chasing endless edge cases, test for this feature programmatically.
|
Thanks for the review. I have rebased and made the suggested changes. All additional feedback is welcome. 🙂 |
|
Thanks now looks great 👍 |
gaborbernat
approved these changes
Jan 10, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, this function checked against a whitelist of OSes that were
known to be a problem. This did include Fedora, which also fails this
test. Rather than chasing endless edge cases, test for this feature
programmatically.