Skip to content

Conversation

@dbast
Copy link
Contributor

@dbast dbast commented Feb 5, 2025

While the two previous PRs focused on getting the tests running on public Github runners and updating the supported Python + poetry versions, this PR now fully focuses on code linting + formatting.

This PR:

  • Delegates all the linting to pre-commit via various hooks (same tools as before but managed by pre-commit)
  • Lets pre-commit manage the hooks -> reduces the dev dependencies in pyproject.toml to mypy*, pytest*, pre-commit and safety.
  • Removes darglint as deprecated / discontinued
  • Updates all the remaining dev dependencies + regenerates poetry.lock
  • Adds a workflow to run pre-commit for PRs
  • Updates Makefile targets so that `make help works
  • Consolidates the many linting + formatting Makefile targets into just only make pre-commit ... the exit code + shown diff tell if it changed something, so that there is now no difference between formatting or linting... just one single make target... thats how many python projects operate nowadays.
  • Fixes/handles all pre-commit findings

green run can be seen here https://github.com/dbast/hyperliquid-python-sdk/actions/runs/13162247128/job/36733665922

While the two previous PRs focused on getting the tests running on public
Github runners and updating the supported Python + poetry versions, this
PR now fully focuses on code **linting** + **formatting**.

This PR:
* Delegates all the linting to pre-commit via various hooks (same tools
  as before but managed by pre-commit)
* Lets pre-commit manage the hooks -> reduces the dev dependencies in
  pyproject.toml to mypy*, pytest*, pre-commit and safety.
* Removes darglint as deprecated / discontinued
* Updates all the remaining dev dependencies + regenerates poetry.lock
* Adds a workflow to run pre-commit for PRs
* Updates Makefile targets so that `make help works
* Consolidates the many **linting** + **formatting** Makefile targets
  into just only `make pre-commit` ... the exit code + shown diff tell
  if it changed something, so that there is now no difference between
  formatting or linting... just one single make target... thats how many
  python projects operate nowadays.
* Fixes/handles all pre-commit findings
Copy link
Contributor

@traderben traderben left a comment

Choose a reason for hiding this comment

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

Thank you for putting this all together. It mostly looks good to me, but I have a couple of concerns. The entire pre-commit hook seems fast enough, but I think I would find it helpful to be able to just run part of it. For example, sometimes I am making a change that introduces some type errors and I'd prefer to just run make check and only have that output instead of needing to filter out other output that I'm not interested in. Also the tests are quite fast, do you think it makes sense to also include them in the precommit hook?

Also, how hard would it be to have the pre-commit hook be check-only by default? I've had bad experiences with tools making edits I don't want/expect and would prefer to explicitly call it with --fix or something when I'm ready for changes.

@dbast
Copy link
Contributor Author

dbast commented Feb 6, 2025

@traderben Updated the pre-commit make target so that running single hooks is possible via e.g. make pre-commit hook=black (the inverse is also possible SKIP=pyupgrade make pre-commit).

Regarding hooks changing code: almost all hooks either only complain (flake8, bandid, pylint, mypy, schema-checks) or only change spacing/newlines (isort, black, ymlfmt). The only single hook that really transforms/changes code is pyupgrade: It super reliable, but we can also disable it via putting it behind stages: [manual]. Though it is a design goal of pre-commit to not specifically differentiate between a fix/check mode, by always telling via the overall exit code and/or shown diff that actually something changed or individual hooks failed... that makes it much simpler

@traderben
Copy link
Contributor

@traderben Updated the pre-commit make target so that running single hooks is possible via e.g. make pre-commit hook=black (the inverse is also possible SKIP=pyupgrade make pre-commit).

Regarding hooks changing code: almost all hooks either only complain (flake8, bandid, pylint, mypy, schema-checks) or only change spacing/newlines (isort, black, ymlfmt). The only single hook that really transforms/changes code is pyupgrade: It super reliable, but we can also disable it via putting it behind stages: [manual]. Though it is a design goal of pre-commit to not specifically differentiate between a fix/check mode, by always telling via the overall exit code and/or shown diff that actually something changed or individual hooks failed... that makes it much simpler

Ok, I'm willing to give this approach a try. Looks like I merged someone else's PR that doesn't pass the linter so will merge your PR as is and fix the lint issues separately

@traderben traderben merged commit c2837c3 into hyperliquid-dex:master Feb 6, 2025
2 of 3 checks passed
@dbast dbast deleted the lint branch February 9, 2025 17:15
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.

2 participants