bpo-36624: Add platform constants in test.support#13648
Conversation
| 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.
I'm not sure that this adds a lot of clarity. Could you remove that?
There was a problem hiding this comment.
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 |
skrah
left a comment
skrah
left a comment
There was a problem hiding this comment.
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. |
7 participants
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_statandtest_subprocess.https://bugs.python.org/issue36624