Klipper3d / klipper

Klipper is a 3d-printer firmware
GNU General Public License v3.0
8.99k stars 5.18k forks source link

Pep8 compliance #6425

Closed eoyilmaz closed 6 months ago

eoyilmaz commented 6 months ago

Hi there, this is my first PR... Nothing fancy here, just updated formatting of the klippy.klippy and klippy.gocde modules to comply with PEP8 and remove some old style string formatting, to prepare for full Python 3 compliance... Just saw the contributing guidelines, I think I didn't follow your commit message format, please let me know if I need to amend my commit message.

JamesH1978 commented 6 months ago

Thank you for submitting a PR, pleas refer to point 3 in "What to expect in a review" in https://github.com/Klipper3d/klipper/blob/master/docs/CONTRIBUTING.md and provide a signed off by line.

Please also refer to the errors in your code as your submission does not pass the build checks.

Thanks James

eoyilmaz commented 6 months ago

I messed up a little, sorry about that. But the commit messages are updated and the line widths are corrected. In anyways, if you don't want to merge this, I can do it from scratch with a clean branch 👍

eoyilmaz commented 6 months ago

My changes caused an import error... working on it...

eoyilmaz commented 6 months ago

Interesting... Python 2 is complaining about a standard library import:

   + /home/runner/work/klipper/klipper/ci_build/python2-env/bin/python klippy/klippy.py --import-test
  Traceback (most recent call last):
    File "klippy/klippy.py", line 17, in <module>
      import configfile
    File "/home/runner/work/klipper/klipper/klippy/configfile.py", line 6, in <module>
      import sys, os, glob, re, time, logging, configparser, io
  ImportError: No module named configparser
  Error: Process completed with exit code 1.
wlhlm commented 6 months ago

See https://github.com/Klipper3d/klipper/commit/c56c34fa1c4c8bf7e10448804604edf600fdefa0. The import of the util module has to come before configfile since the former does some hacky stuff to make the latter work in py2.

eoyilmaz commented 6 months ago

Is there anything else that I need to update in this PR so that it can be merged?

KevinOConnor commented 6 months ago

Thanks. However, per our development guidelines we don't merge whitespace changes, except as from the "established owner" of a particular module (this is mentioned briefly at https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review ). The primary reason for this, is that whitespace changes make it harder to maintain and perform merges, they make tools like git bisect harder to use, and they can hide functional changes. (They can also sometimes lead to "style wars" and "bikeshedding".)

-Kevin

flowerysong commented 6 months ago

Note that even if the project decided it wanted to move in this direction the majority of your proposed changes are unrelated to PEP8 compliance, e.g. the switch from single quotes to double quotes. Per https://peps.python.org/pep-0008/#string-quotes:

In Python, single-quoted strings and double-quoted strings are the same. This PEP does not make a recommendation for this.

Also things like https://github.com/Klipper3d/klipper/pull/6425/files#diff-e66206e2d96cdcb7cbee967ee88c7e31d6308a6b05af6aace4610400868b1643L109-R180 ; the previous formatting is allowed by PEP8 (see https://peps.python.org/pep-0008/#indentation).

You appear to have reformatted the files to match your personal style preferences (or run an automated code formatter like black against them), rather than just making the minimal changes required for PEP8 compliance. That makes a change like this much harder to review than it needs to be (as does mixing in non-cosmetic changes) and more likely to be rejected by the target project.

eoyilmaz commented 6 months ago

Sure sure, that makes all sense. I'm closing this PR 👍