On 10/3/2018 1:46 AM, Neil Schemenauer wrote:
> On 2018-10-02, Michael Felt wrote:
>> I am sorry, for myself obviously - but also for Python. Obviously, I am
>> doing it all wrong - as I see lots of other issues being picked up
>> immediately.
> I'm not sure that's the case. There are a lot of PRs or bugs that
> sit there without getting reviews. The problem is that few (or no)
> core developers get paid to work on Python. So, the time they spend
> is motivated by their specific "itch". Getting reviews on any PR is
> difficult, even for core developers. In their case, they have to
> option of forcing the issue, I guess.
>
> This is a problem we should try to deal with somehow. Turning off
> valuable contributors like you is bad. I'm not sure how to do it
> though. At the core Python sprint in September there was some talk
> about how CPython developers might get funding. Maybe that could
> help deal with the backlog of reviews required.
>
>> And, while you may not give a damn about anything other than Windows,
>> macos and/or Linux - there are other platforms that would like a stable
>> Python.
> There is probably some truth in not caring about other platforms.
> The problem from the reviewer perspective is the question of "what
> is the potential downsides of this PR vs what are the benefits?".
> The safest thing is to not approve the PR. No core developer wants
> to be the person who broke CPython. You must admit, AIX is an
> extremely niche platform at this point. I bet if you picked 1000
> software developers at random, it would be likely that zero of them
> have ever used AIX. So, it's not that we don't care at all about
> AIX but that the cost/benefit equation makes accepting AIX specific
> changes more difficult.
Nods. However - this is a chicken/egg issue (imho). AIX is seen a weak
platform because noone has ever tackled these. When I started on this I
had never expected to have found a resolution to them all.
Platforms have differences and when the tests miss that difference that
the tests give a false result. e.g., one accepted PR was because AIX
libc printf() output for printf(NULL) is "" while other platforms output
"(null)".
>
> One specific suggestion I have about your PR is to try to make your
> changes not AIX specific. Or at least, make the AIX checking as
> localized as possible. So, as an example, in test_uuid you have:
>
> _notAIX = not sys.platform.startswith("aix")
a) I thought/hoped this was better practice and performance - calling
sys.platform.startswith("aix")only once, rather than X times.
b) more maintainable (e.g., change to not platform.system()
c) iirc - this got changed to AIX = ...., and throughout the test is "if
not AIX"...
>
> then later in the module you check that flag. While that is the
> most direct approach to fixing the issue and making the test pass,
> it is not good for the long term maintainability of the code. You
> end up with boolean flags like _notAIX spread about the logic. Over
> time, code like that becomes a nightmare to maintain.
>
> Instead, I would suggest test_uuid is making platform specific
> assumptions that are not true on AIX and possibly other platforms.
> So, do something like:
>
>
> _IS_AIX = sys.platform.startswith("aix")
better name.
>
> _HAVE_MACADDR = (os.name == 'posix' and not _IS_AIX)
AIX has MACADDR, but formatted with '.' rather than ':' and uses a
single hex-digit when value between dots is < 16 (decimal)
>
> @unittest.skipUnless(_HAVE_MACADDR, 'requires Posix with macaddr')
> def test_arp_getnode(self):
> ...
>
> The _HAVE_MACADDR test is relatively simple and clear, does this
> platform have this capability. Later in the code, a check for
> _HAVE_MACADDR is also quite clear. If someone comes along with
> another platform that doesn't support macaddr, they only have to
> change one line of code.
>
> This kind of capability checking is similar to what happened with
> web browsers. In that case, people discovered that checking the
> User Agent header was a bad idea. Instead, you should probe for
> specific functionality and not assume based on browser IDs. For the
> macaddr case, is there some way to you probe the arp command to see
> if supports macaddr?
I suppose if someone had written the original test with "check program
to see if ..." it would have worked already.
I am trying to get current tests to work with minimal changes.
I am certainly not "blaming" anyone for not knowing this unique behavior
of this platform. Before debugging this I did not know of the difference
either. I agree that wherever a generic resolution is possible - it
should be done. However, as an "outsider" I do not feel empowered enough
to propose such a change to existing (and well proven for other
platforms) tests.
> That way your test doesn't have to include any
> AIX specific check at all. Further, it would have some hope of
> working on platforms other than AIX that also don't support macaddr
> but are POSIX and have 'arp'. The code could be something like:
>
> _HAVE_MACADDR = False
> if os.name == 'posix':
> if <check arp to see if it supports macaddr>:
> _HAVE_MACADDR = True
>
> Hope that is helpful.
All feedback and constructive criticism is helpful. Thank you for taking
the time to share!
>
> Neil
_______________________________________________
Python-Dev mailing list
Python-Dev@python.org
https://mail.python.org/mailman/listinfo/python-dev
Unsubscribe:
https://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
