In itself, the code clean up you have done is a good thing in the sense
that you re-organized things and in my understanding, they look good now.
In some of the teams I've been granted to work, there was a rule stating
that whenever a dev would work on an enhancement/bugfix, he would create a
separate ticket that would track all code refactoring related to his work.
The strategy is quite different here and the suggestion was to have your
refactoring be part of an enhancement/bugfix. The most valuable argument in
favor of all the current reviewers is Guido van Rossum's comment on the
effect of having git blame made harder.
For some of the projects I'm currently working on, we have code cleanup
teams where members are affected expressly to refactoring code. We might
end up needing such things in the future but for now, it is wiser to leave
the code as is and focus on fixing actual problems.
*Q::Drone's Co-Founder and Co-CEO*
*Hervé "Kyle" MUTOMBO*
*Envoyé depuis le web.*
On 2018-06-27 00:02, Guido van Rossum wrote:
> And TBH a desire to refactor a lot of code is often a sign of a
> relatively new contributor who hasn't learned their way around the code
> yet, so they tend to want to make the code follow their understanding
> rather than letting their understanding follow the code.
...or it could be that the code is written the way it is only for
historical reasons, instead of being purposely written that way.
On 2018-06-26 21:43, Mark Shannon wrote:
This actually looks close to Victor Stinner's bpo-29259. But instead of
storing the function pointer in the class, you're storing it in the
One concern that I have is that this might lead to code duplication. You
require that every class implements its own specialized
_FOO_FastcallKeywords() function. So you end up with
_PyFunction_FastCallKeywords(). If I want to implement a similar class
myself, I have to reinvent that same wheel again. With PEP 580, I
replace all those _FOO_FastCallKeywords() functions by one
PyCCall_FASTCALL() function. Admittedly, my PyCCall_FASTCALL() is more
complex than each of those _FOO_FastcallKeywords() individually. But
overall, I think that PEP 580 leads to simpler code.
Second, you still have a performance problem for methods. You made sure
that the method optimizations in the Python bytecode interpreter
continue to work, but method calls from C will be slowed down. I don't
know to what extent and whether it really matters, but it's something to
On https://github.com/python/cpython/pull/7909 I encountered friction
for a PR which I expected to be uncontroversial: it just moves some code
without changing any functionality.
So basically my question is: is there some CPython policy *against*
refactoring code to make it easier to read and write? (Note that I'm not
talking about pure style issues here)
Background: cpython has a source file "call.c" (introduced in
https://github.com/python/cpython/pull/12) but the corresponding
declarations are split over several .h files. While working on PEP 580,
I found this slightly confusing. I decided that it would make more sense
to group all these declarations in a new file "call.h". That's what PR
7909 does. In my opinion, the resulting code is easier to read. It also
defines a clear place for declarations of future functionality added to
"call.c" (for example, if we add a public API for FASTCALL). Finally, I
added/clarified a few comments.
I expected the PR to be either ignored or accepted. However, I received
a negative reaction from Inada Naoki on it.
I don't mind closing the PR and keeping the status quo if there is a
general agreement. However, I'm afraid that a future reviewer of PEP 580
might say "your includes are a mess" and he will be right.
Just a reminder that PEP 576 still exists as a lightweight alternative
to PEP 575/580. It achieves the same goals as PEP 580 but is much smaller.
Unless there is a big rush, I would like to do some experiments as to
whether the new calling convention should be
typedef (*callptr)(PyObject *func, PyObject *const *stack,
Py_ssize_t nargs, PyObject *kwnames);
or whether the increased generality of:
typedef (*callptr)(PyObject *func, PyObject *const *stack,
Py_ssize_t nargs, PyObject *kwnames,
PyTupleObject *starargs, PyObject *kwdict);
is a worthwhile enhancement.
An implementation can be found here:
Thanks to a PR from Ammar Askar we now run Python under lcov as part of the
code coverage build. And thanks to codecov.io automatically merging code
coverage reports we get a complete report of our coverage (the first
results of which can now be seen at https://codecov.io/gh/python/cpython).
And funny enough the coverage average changed less than 1%. :)
On 2018-06-26 13:11, Ivan Pozdeev via Python-Dev wrote:
> AFAICS, your PR is not a strict improvement
What does "strict improvement" even mean? Many changes are not strict
improvements, but still useful to have.
Inada pointed me to YAGNI
(https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it) but I
disagree with that premise: there is a large gray zone between
"completely useless" and "really needed". My PR falls in that gap of
"nice to have but we can do without it".
> You may suggest it as a supplemental PR to PEP 580. Or even a part of
> it, but since the changes are controversial, better make the
> refactorings into separate commits so they can be rolled back separately
> if needed.
If those refactorings are rejected now, won't they be rejected as part
of PEP 580 also?
On 2018-06-26 13:54, Ivan Pozdeev via Python-Dev wrote:
> This is exactly what that the YAGNI principle is about, and Inada was
> right to point to it. Until you have an immediate practical need for
> something, you don't really know the shape and form for it that you will
> be the most comfortable with. Thus any "would be nice to have"
> tinkerings are essentially a waste of time and possibly a degradation,
> too: you'll very likely have to change them again when the real need
> arises -- while having to live with any drawbacks in the meantime.
It is important to clarify that this is exactly what I did. I *have* an
implementation of PEP 580 and it's based on that PR 7909.
I just think that this PR makes sense independently of whether PEP 580
will be accepted.
> So, if you suggest those changes together with the PEP 580 PR
That sounds like a bad idea because that would be mixing two issues in
one PR. If I want to increase my chances of getting PEP 580 and its
implementation accepted, I shouldn't bring in unrelated changes.
To put it in a different perspective: if somebody else would make a PR
to one of my projects doing a refactoring and adding new features, I
would ask them to split it up.
On 2018-06-26 13:54, INADA Naoki wrote:
> Real need is important than my preference. If it is needed PEP 580, I'm OK.
Of course it's not needed. I never claimed that it was.
I think it's *nice to have* right now and slightly more *nice to have*
when changes/additions are made to call.c in the future.