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

CI, linting, and tests #13

Merged
merged 2 commits into from
Nov 22, 2023
Merged

CI, linting, and tests #13

merged 2 commits into from
Nov 22, 2023

Conversation

chanind
Copy link
Collaborator

@chanind chanind commented Nov 21, 2023

This PR adds black, pytest, and ruff to dev.in, and adds a simple github actions script to run make style and make test. In addition, I ran make fmt before committing, which reformatted a ton of code, thus the changes seen here.

Some other things to note:

  • I changed the test style to use pytest instead of unittest, which matches what's in the Makefile. Also I feel pytest is a better test runner than unittest (open to discussion on this if there's different views)
  • I didn't commit the updated dev.txt and main.txt, since they change depending on which system runs the make script which will cause conflicts depending on who ran the generation script last.
  • I didn't optimize the github action performance, so it's very slow during dependency install right now. We can improve that by caching dependencies between builds. I'll do that in a separate PR after this is merged.
  • I didn't set up isort, but it might be handled anyway by ruff. We can revisit that later if needed.
  • I didn't set up Pyright type checking. That may require some code changes to get everything compliant, and this PR is already large enough IMO. We can enable Pyright in a follow-up PR.

@chanind chanind requested review from dtch1997 and aengusl November 21, 2023 14:47
@aengusl aengusl merged commit 7e08718 into dev Nov 22, 2023
1 check passed
@aengusl aengusl deleted the ci-linting-tests branch November 22, 2023 14:01
@dtch1997
Copy link
Owner

LGTM

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