python / cpython

The Python programming language
https://www.python.org
Other
62.31k stars 29.93k forks source link

IDLE editor file minor refactoring #88060

Open 919475b3-36a2-4cd7-997c-9c38f05f93c7 opened 3 years ago

919475b3-36a2-4cd7-997c-9c38f05f93c7 commented 3 years ago
BPO 43894
Nosy @terryjreedy, @merwok, @E-Paine
PRs
  • python/cpython#25485
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = 'https://github.com/terryjreedy' closed_at = None created_at = labels = ['expert-IDLE', '3.8', '3.9', '3.10'] title = 'IDLE editor file minor refactoring' updated_at = user = 'https://github.com/E-Paine' ``` bugs.python.org fields: ```python activity = actor = 'epaine' assignee = 'terry.reedy' closed = False closed_date = None closer = None components = ['IDLE'] creation = creator = 'epaine' dependencies = [] files = [] hgrepos = [] issue_num = 43894 keywords = ['patch'] message_count = 4.0 messages = ['391449', '391727', '391735', '391736'] nosy_count = 3.0 nosy_names = ['terry.reedy', 'eric.araujo', 'epaine'] pr_nums = ['25485'] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = None url = 'https://bugs.python.org/issue43894' versions = ['Python 3.8', 'Python 3.9', 'Python 3.10'] ```

    919475b3-36a2-4cd7-997c-9c38f05f93c7 commented 3 years ago

    Despite being large, the PR is mostly trivial changes (e.g. changing indentation). The main changes worth noting (and hence the reason for this issue) are: 1) the renaming of ResetColorizer to reset_colors, ResetFont to reset_font, RemoveKeybindings to remove_keybindings and ApplyKeybindings to apply_keybindings. 2) removal of some potentially unneeded imports 3) renaming of the tokeneater args to be lowercase

    I chose reset_colors (as proposed in PR-22682) rather than reset_colorizer because we are reconfiguring the Text, code context and line numbers, rather than only dealing with the colouriser.

    merwok commented 3 years ago

    Hello and thanks for wanting to contribute to Python!

    In the CPython project, stylistic changes are not made by themselves as a matter of course (accross the whole codebase or in a specific module): among reasons, these obfuscate file history, need reviewer time that takes away from other changes, and can introduce subtle bugs. Or in short: if it’s not broken, don’t fix it!

    This is different from other projects, especially those that have a smaller codebase, much less history than Python, and can agree on using some automated tool for trivial formatting changes.

    So if the changes here are not motivated by a bugfix or a new features, I think they should not be accepted.

    terryjreedy commented 3 years ago

    This started as an extraction and revision of changes in Tal's PR, one of which appeared to be an intended behavior change, that I requested there. Paine may have expanded the scope more than I intended. Will look soon.

    919475b3-36a2-4cd7-997c-9c38f05f93c7 commented 3 years ago

    I agree with your points and understand that there is good reason for the PR to be rejected. If Terry decides it's too big (I did do a lot more than what was originally in PR-22682), I am more than happy to close this issue.