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

Add test and lint framework #199

Merged
merged 5 commits into from
Sep 2, 2024
Merged

Conversation

jamescurtin
Copy link
Collaborator

In preparation for making a few more contributions to #165, I first wanted to propose a framework for linting and auto-formatting the codebase.

I'm proposing ruff, based on it being a drop-in replacement for all of the standard linting tools you might use, e.g. Flake8, Black, isort, pydocstyle, etc. as well as being significantly faster to run.

Note that in this PR, I only enable formatting. I do so for two reasons:

  • I want to make sure that ruff is an agreeable linting and formatting tool before putting in too much time
  • This changes introduces no semantic changes and I don't want to mix in linting fixes, which would be effectively unreviewable given the size of the diff.

If this approach is amenable, I will follow up with a second PR to enable ruff's linting.


Note that formatting and linting can be performed via a docker compose command, but will also be run automatically if you're developing the project in VSCode. As a general principle, the CI check that is added to Github Actions runs via the same docker compose command that is run locally to ensure there isn't drift in environment or configuration between local development and CI.


Reviewing

This PR is quite large, but the majority of changes are auto-generated, so should be reviewed by commit. Note that this will create a bunch of merge conflicts if you have any in-flight changes you'd like to merge soon. If that's the case, I can hold off on merging this until afterward those changes land. It's easier for me to rebase and re-generate the last commit (where the actual code formatting happens), than for someone else to deal with merge conflicts 😄

  • bfeb119: Adds the scaffolding to be able to run ruff. As part of adding the ruff dependency, this commit made the following related changes:
    • Introduces a new requirements-test.txt file to be able to add additional packages for testing/linting without increasing the surface area of what is bundled into the production image
    • Introduces a utility script to make it easier to add new dependencies or upgrade dependency versions.
  • cc108b7: Resolves a typo that resulted in a Python file being uninterpretable, which meant that the formatter was unable to run against the project.
  • 8b8ec22: Introduces a Github Actions check that runs the formatter in check-only mode, failing if unformatted code is detected.
  • 90195a2: Updates the VSCode workspace settings file. For those who develop on this project in VSCode, it should hopefully make the formatting process automated and less toilsome
  • f537463: This is the big one: all changes are auto-generated by running ruff format .... These changes are formatting only and have no semantic meaning. The code before and after should generate the same AST[0]

This CI build run on my fork shows that the new format check is currently passing.

[0] Technically ruff can modify the AST, but in a trivial way.

@jamescurtin jamescurtin changed the title Add test and lint Add test and lint framework Sep 1, 2024
@iragm
Copy link
Owner

iragm commented Sep 1, 2024

I want to make sure that ruff is an agreeable linting and formatting tool before putting in too much time

Great. A linter has been on my todo list for about 4 years now.

This changes introduces no semantic changes and I don't want to mix in linting fixes, which would be effectively unreviewable given the size of the diff.

Right.

Note that this will create a bunch of merge conflicts if you have any in-flight changes you'd like to merge soon.

This is an excellent time to merge this in. I have nothing in the pipe and I am focusing on making some tutorial videos in what little time I have, so no code changes are planned on my end for the next couple weeks.

I do use VSCode, and to my knowledge there aren't any other people developing on this project. I'll merge this one on Tuesday when I get back if that's OK. It looks like it's all autogenerated and can't possibly break anything, it's just too large for me to review every line.

@jamescurtin
Copy link
Collaborator Author

Excellent! If this goes well, I have the next set of changes queued to enable the linter. They can be previewed here: jamescurtin/fishauctions@add-test-and-lint...jamescurtin:fishauctions:add-linting

@iragm
Copy link
Owner

iragm commented Sep 1, 2024

This is looking great, you've cleaned up years worth of ugly code!

The bare except stuff needs to be gone over in more detail, most of them should be ValueError, not Exception.

A lot of the try except code can be removed altogether, for example the UserData is now always created as soon as the user is saved, so it can be accessed at user.userdata without a try except block.

There's also a lot of places where a dict.get() with a default value is a lot cleaner than try except.

I was waiting on more tests to make these changes, I just haven't had a chance to put everything together yet.

@iragm iragm merged commit 98b3383 into iragm:master Sep 2, 2024
3 checks passed
@jamescurtin jamescurtin mentioned this pull request Sep 2, 2024
@jamescurtin jamescurtin deleted the add-test-and-lint branch September 3, 2024 01:18
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