What type of issue to create on github for a simple refactor of Lib/calendar.py

Ideas
1

I would like to do a small refactor of cpython/calendar.py at main · python/cpython · GitHub, nothing major, but there are many small things that could be done to increase the readability and remove bad practices (I see multiple times an alias for v.append being used (explicit > implicit), 1 letter variables, inconsistency use of constants, etc).

However, I am not sure if “Issue: Feature or enhancement” is the correct type of issue, since my planned changes seem rather minimal compared to what is written in the template. I could definitely write out all the changes I am planning to do before making the PR if that is preferred. Also please let me know if simple refactors are not welcomed.

2

I don’t think it matters much if you call it a feature or enhancement. Sometimes it’s not clear at first. It can always be changed.

In general we don’t accept simple refactorings. The chance for breakage is too high, especially with old modules.

6 Likes
3

Simple refactors are not welcomed.

2 Likes
4

Refactoring can be acceptable if it serves a proven purpose. For example, it may be beneficial to refactor a certain passage in order to add new features or to apply performance optimisations. A prerequisite for any kind of refactoring is of course a significant increase in code coverage. For example, Irit Katriel has done a few targeted refactorings in the assembler/compiler, and the exception handling code. I’ve done some refactoring myself, but only after studying the code in question, often for several months; such studies include trying to find new possible paths through the code, finding untested corner cases, etc. It is a tedious task not to be taken lightly.

Refactoring can be a very useful tool if wielded correctly. If overused, it can significantly increase the risk of introducing new bugs, as Eric already noted.

6 Likes
5

Naming a bound method to avoid repeated lookup and bound-method creation is a standard practice and not something to be undone.

2 Likes
6

I see, Terry, I did not think of that.

Thanks everyone for your responses.

7

Refactoring can be acceptable if it is done by the main maintainer of this code who has maintained it for several years and continue to will maintain it in future. For example, Terry is the author of 90% to 99% of changes in IDLE in the last years. I perhaps wouldn’t do the refactoring he did (a lot of module renaming), but if it makes the IDLE code better for him, that’s all that matters.

A refactoring PR from a casual contributor should be rejected. It only drains core developers time for reviewing and has a risk of introducing bugs or other regressions. See also Chesterton’s fence. If you see something that you do not understand in the code, there may be reasons for it.

2 Likes
8

The 3 major refactorings I have done, with test for code changes, all removed major confusions and pain points and allowed improvements that would not have happened otherwise. They involved another person, were thought about for at least a year, and benefited most everyone who worked on IDLE thereafter.

There are about 4 open calendar issues so there would not seem to be a crying need to redo the code.

Correction: while I have reviewed all the substantive code changes in the last few years, I have written at most 50%, not 90+% .

1 Like