Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates KMK's code style in line with Black's double quotes #571

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

LukeDRussell
Copy link
Contributor

@LukeDRussell LukeDRussell commented Sep 4, 2022

In #561 I raised the possibility of fully complying with black. The maintainers tended to agree that double-quotes are fine, as long as it's consistent everywhere.

To that end, I've run black and isort across the entire project. Then got flake8 and unittests to all pass.

IMO contributors should use block exclusions for keymaps, et al rather than entire files. This should help newcomers see a consistent style for the parts of KMK they look at most - namely docs/, keymaps/ and boards/**/main.py

I also took the opportunity to move the remaining isort configs into pyproject.toml. The remaining flake8 configs into a named file. This will probably not move into pyproject.toml until Python 3.11 which adds a toml parser to the standard library.

I've also added a git_ignore_revs file so git blame won't blame me for style changes ;) Seems to work in the Github diff UI natively.

@LukeDRussell LukeDRussell changed the title Minimizes non-standard style options [DRAFT] Updates KMK's code style Sep 4, 2022
isort config updated with latest libraries documented by Adafruit.
@LukeDRussell LukeDRussell changed the title [DRAFT] Updates KMK's code style Updates KMK's code style in line with Black's double quotes Sep 7, 2022
@LukeDRussell
Copy link
Contributor Author

So this PR seems ready to me. All formatting and tests pass on Win11 with Python 3.10 and Fedora-WSL with 3.9.
I've done my best to not change any content with this PR, but there were a handful of long-lined comments (that black won't touch) that flake8 didn't like. Generally I split the lines manually, in some cases I removed "fluff" words.

@LukeDRussell LukeDRussell marked this pull request as ready for review September 7, 2022 03:01
@kdb424 kdb424 requested a review from xs5871 September 7, 2022 03:10
Copy link
Contributor

@kdb424 kdb424 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think this covers my thoughts. Obviously, all PR's will need to conform to this, which will make any of them fail, ect, but other than notes, if pushing through pure black formatting is considered a better approach, then those comments are all that I have before I won't block a merge.

boards/keebio/iris/kb.py Outdated Show resolved Hide resolved
boards/keebio/iris/kb_converter.py Outdated Show resolved Hide resolved
boards/keebio/iris/main.py Outdated Show resolved Hide resolved
boards/keebio/iris/main.py Outdated Show resolved Hide resolved
boards/keebio/iris/main.py Outdated Show resolved Hide resolved
@@ -1,58 +0,0 @@
[flake8]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just making sure it is intentional to delete this file. I presume that just "taking black defaults" is what you expect to do, including ignoring all of the convenient exceptions? Not refuting it, just making sure that we all know what the intent here is.

Copy link
Contributor Author

@LukeDRussell LukeDRussell Sep 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional.
Only Flake8 remains that doesn't use pyproject.toml. The way I was reading that is they were waiting for a parser to make the standard library (3.11) or for a clear standard to emerge.
Putting flake8 config in it's own file makes it clear (at least to me) that no new configs should be declared in the legacy setup.cfg, preferring pyproject.toml where possible.
Something for the contributing.md, will add if you agree.

user_keymaps/kdb424/nyquist_r2.py Show resolved Hide resolved
@kdb424
Copy link
Contributor

kdb424 commented Sep 7, 2022

Other thing that totally got left out. Docs. They are all still single quote, and need to be updated to reflect the new style, or we cause confusion for users.

Changes requested in PR571
Doc changes still to come.
@kdb424
Copy link
Contributor

kdb424 commented Sep 7, 2022

Once docs are resolved, I won't block merger, and allow any other maintainer to sign off and merge.

@LukeDRussell
Copy link
Contributor Author

To Do:

  1. Reformat embedded code in docs/
  2. Expand docs/contibutors.md with preference to use pyproject.toml for configurations.
  3. Update Readme.md to remove the sentence about quotes.

@LukeDRussell
Copy link
Contributor Author

LukeDRussell commented Sep 14, 2022

I've not forgotten this PR, I just haven't been able to give it my attention. Next week I'll be between $Jobs so I'll be able to get it across the line.

@klardotsh
Copy link
Member

When you get back (no rush!) let's also update the README to match, since the mainline readme still talks about this vestigial choice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants