-
-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
bpo-36624: Add platform constants in test.support #13648
Conversation
@@ -4862,7 +4862,7 @@ def test_c_context_errors(self): | |||
for attr in ('prec', 'Emin', 'Emax', 'capitals', 'clamp'): | |||
self.assertRaises(OverflowError, setattr, c, attr, int_max+1) | |||
self.assertRaises(OverflowError, setattr, c, attr, -int_max-2) | |||
if sys.platform != 'win32': | |||
if not MS_WINDOWS: |
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.
I'm not sure that this adds a lot of clarity. Could you remove that?
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.
It is not about just this one file. The request was, as much as possible, to use constants in the tests rather than different approaches such as 'sys.platform.startswith("win")' and sys.platform == 'win32'.
Compare with (if pasting images works)
Otherwise:
Look at this file (Lib/test/_test_multiprocessing.py, https://github.com/python/cpython/pull/13648/files#diff-b046ab474480855fb4a01de88cfc82bb) - where, for windows both a local constant (WIN32) and sys.platform {==|!=} 'win32' is used.
imho - this adds clarity to the whole.
I'm a bit uncertain about the benefit of these in |
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 for asyncio part
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.
Could you take out test_decimal? I don't quite like this change.
When you're done making the requested changes, leave the comment: |
On Wed, May 29, 2019 at 02:43:16AM -0700, Michael Felt wrote:
imho - this adds clarity to the whole.
I don't think so: Now I have to wonder which of the different idioms hides behind MS_WINDOWS.
And in other project's setup.py files I still need sys.platform, so now there's even one additional way to accomplish the same thing.
|
As I tried to show above - it is not just one file. IMHO - this needs to be unanimous. Without that there is no future where a code review could say - use constants. And, yes, some platforms such as bsd* and solaris do not have constants - yet. "My" intent is that they are also done - asap. Where there is a will, there is way. |
So, Python does nothing? Doing this in "test" gives "the example" on how it should be done. As to the areas outside of test, and even within test (e.g., for bsd*) I am willing to make this "my mission" to bring uniformity wherever possible. This was not even my idea :) - this was a suggestion on how I could contribute something to the whole. Again, if not clear before - I do not see it stopping here - but would clearly welcome direction on where to set priorities. It is not my call - I am not in python-dev mailing list - so I will not see a discussion there. If the group feels this is wrong - I have no direct interest in any decision made - other than my attempt - read desire - to contribute to Python. Regards, |
Considering this is a rather minor change, discussion about it should be done on the bug tracker, bpo-36624 (except when discussion specifics of the implementation in this PR). @skrah, your input would be helpful there. @aixtools, you've made your point rather clearly, let's see what some other devs think about this now that there's a PR to look at. |
@taleinat I'm aware of the bug tracker. |
@skrah, I have no doubt about that :) I wanted to stop the discussion from continuing here where it could be missed by those following the issue on the bug tracker, so I wrote to clarify for everyone. |
@taleinat, FWIW, I disagree wih your assertion that this is a "rather minor change". See my comments on the bug tracker issue. |
The discussion here and on the issue is clearly going towards not making this change, so I'm closing this PR. |
Add platform constants in
Lib/test/support.py
:support.MS_WINDOWS
,support.AIX
, etc.This is a clean rebase of the original PR by @aixtools, GH-12843.
I've also fixed two minor errors which broke
test_stat
andtest_subprocess
.https://bugs.python.org/issue36624