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

Improve formatting #236

Merged
merged 9 commits into from
Aug 5, 2021
Merged

Improve formatting #236

merged 9 commits into from
Aug 5, 2021

Conversation

sommersoft
Copy link
Collaborator

Fixes #225.

This is a large one, so apologies for the heavy review load.

Changes include the following:

  • Ran Black on all Python files

  • Ran Pylint and addressed most issues. There are some pylint: disable markers that can be cleaned up, but would be better done outside this PR.

  • Actions workflow changes:

    • Updated Python version to 3.9 in all workflows
    • Added pre-commit config and workflow to run black and pylint
  • Beyond automated tests, the following were tested (the rest are currently hard to test):

    • adabot/update_cp_org_libraries.py (with -o)
    • adabot/circuitpython_library_patches.py (with --dry-run & --dry-run --local)
    • adabot/circuitpython_library_download_stats.py (with -o)
    • adabot/lib/assign_hacktober_label.py

I've got a small list of new issues to address some of the pylint: disable markers, and some other stuff I came across. I'll get those filed soon.

@evaherrada
Copy link
Collaborator

Oh wow, thanks so much for doing this. Yeah, there were an absurd amount of things that needed to be linted here.

Copy link
Collaborator

@evaherrada evaherrada left a comment

Choose a reason for hiding this comment

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

@sommersoft I can't thank you enough for doing this. Last time I checked Pylint had about 700 things that it wanted changed, I can't imagine how long it took.

@evaherrada
Copy link
Collaborator

evaherrada commented Aug 5, 2021

Testing the scripts:

  • arduino_libraries
  • update_cp_org_libraries
  • circuitpython_libraries
  • circuitpython_library_download_stats

Copy link
Contributor

@kattni kattni left a comment

Choose a reason for hiding this comment

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

This is brilliant, thank you so much for doing it! @dherrada is testing it locally to ensure everything is still copacetic.

@sommersoft I had one question below. I noticed you pinned Black, and I wasn't sure if we should be pinning Pylint as well.

@@ -1,3 +1,4 @@
black==21.6b0
packaging==20.3
pylint
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be pinned as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I noticed you pinned Black, and I wasn't sure if we should be pinning Pylint as well.

I didn't pin it since the locally installed module is used to verify that a library is passing with the latest pylint, in lib/circuitpython_library_validators.py::validate_passing_linting.

However, in looking back at changes to libs/cookiecutter, it seems they're all now pinned to 2.7.1 via pre-commit-config. Which brings the question: is validate_passing_linting overachieving by linting with the latest version?

@evaherrada
Copy link
Collaborator

Ok. Just finished running all 4 of the scripts that we can run locally and they all worked as expected.

@kattni
Copy link
Contributor

kattni commented Aug 5, 2021

I'm ok with merging this and if we were supposed to pin Pylint, we can handle it in another PR.

Thank you for testing, Dylan!

Thanks so much for this whole thing, sommersoft!

@kattni kattni merged commit f879b24 into adafruit:main Aug 5, 2021
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.

Repo isn't being linted
3 participants