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

General code quality fixes #92

Merged
merged 23 commits into from
Dec 12, 2021
Merged

General code quality fixes #92

merged 23 commits into from
Dec 12, 2021

Conversation

akx
Copy link
Contributor

@akx akx commented Nov 28, 2021

This PR applies a bunch of general code quality & typing fixes. In that, it may fix some future or corner-case bugs.

👉 I would strongly advise this PR not be squash-merged in case trouble arises down the line; it would make it a lot easier to bisect (not to mention git annotates look better).

In particular:

  • f-strings are used everywhere (since this is 3.7+)
  • dataclasses are used everywhere
  • fixes typing so mypy --strict now passes fully for the codebase
  • all of PyCharm's "Problems" complaints are dealt with
    • methods that needn't be methods are now free functions
  • use of modern py.test features
    • all tests pass, of course

Some future improvements might include:

  • turning PilotBuilder into a function that simply accepts a bunch of optional kwargs and returns a payload
  • turning PilotParser into a function that accepts a response and returns a dataclass
  • changing naming of external interfaces to be PEP8 compliant (PascalCase and snake_case); I didn't do this yet since it would be a semver-major change.
  • adding py.typed since this package should now be soundly typed

Of course please let me know if you want this e.g. split into multiple PRs, or if there's any concerns :)

@sbidy
Copy link
Owner

sbidy commented Nov 30, 2021

@akx awesome changes!! Thank you!
It's a good point to refactor the PilotBuilder and PiloteParser into a clean function, but this will break eventually some integrations. But I will be more readable and usable with functions.

@sbidy
Copy link
Owner

sbidy commented Nov 30, 2021

@all-contributors please add @akx code

@allcontributors
Copy link
Contributor

@sbidy

I've put up a pull request to add @akx! 🎉

@akx
Copy link
Contributor Author

akx commented Dec 1, 2021

@akx awesome changes!! Thank you! It's a good point to refactor the PilotBuilder and PiloteParser into a clean function, but this will break eventually some integrations. But I will be more readable and usable with functions.

@sbidy Thank you!

Sure - those breaking changes could be done in the next semver-major update (0.5.x or 0.6.x maybe?).

Can you approve GH workflows for this PR (first-time contributors need approval) so I can see if there's anything that needs changes lint-wise?

@akx
Copy link
Contributor Author

akx commented Dec 2, 2021

@sbidy I rebased this and fixed the issues that occurred in CI, but I had to edit the workflow file to do so, so you'll need to re-approve :)

@akx
Copy link
Contributor Author

akx commented Dec 7, 2021

@sbidy Fixed the last CI fix. Once more with feeling, as they say! You can see the checks having succeeded over at akx#1 ...

@sbidy
Copy link
Owner

sbidy commented Dec 7, 2021

Yes, hopefully I can get my hands on the PR to merge this in the next days.
Thank you for the CI fix...

@sbidy sbidy merged commit da47c70 into sbidy:master Dec 12, 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.

None yet

2 participants