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

Introduce Ruff linter/formatter and test runner with coverage report #26

Merged
merged 6 commits into from
May 29, 2024

Conversation

K-dash
Copy link
Collaborator

@K-dash K-dash commented May 29, 2024

@ryansurf

This pull request introduces Ruff as a linter/formatter tool and sets up a test runner with coverage reporting.

The following changes have been made:

  • Ruff has been added as a linter/formatter tool to maintain code quality and consistency.
  • The pyproject.toml file has been updated with Ruff configuration settings.
  • Makefile tasks have been added to easily run the Ruff linter (make lint) and formatter (make format).
  • Linter errors have been resolved, and the source code has been updated to adhere to the formatting rules.
  • A test runner configuration has been set up, along with coverage reporting.
  • Makefile tasks have been added to easily run tests (make test) and generate a coverage report (make post_test).
  • The CI configuration (pytest.yml) has been updated to run tests and post the coverage report as a comment on pull requests.
  • A dev-requirements.txt file has been added to specify the development dependencies, including Ruff, pytest, and pytest-cov.

The addition of the dev-requirements.txt file makes it easier for developers to set up the necessary development dependencies by running pip install -r dev-requirements.txt.

Please review!!

@K-dash
Copy link
Collaborator Author

K-dash commented May 29, 2024

@ryansurf
I added a CI (pytest.yaml) to write the coverage report into the PR conversation, but it fails with the following error:

image

I added permissions: write-all to pytest.yaml, but it didn't work.

MishaKav/pytest-coverage-comment#68

It seems that the repository owner needs to change the branch protection settings according to the above link. Could you please handle this?

@K-dash
Copy link
Collaborator Author

K-dash commented May 29, 2024

@ryansurf
#26 (comment) <- I did some research.
It seems that writing to a PR from GitHub Actions is done using the GITHUB_TOKEN, but in the case of PRs from forked repositories, write access cannot (and should not) be obtained.

https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token

Therefore, I specified continue-on-error: true in the process that outputs and generates the coverage report in pytest.yml, so that the CI will succeed (and the PR can be merged) even if it fails. (Unfortunately...😢)

When you, as the owner, submit a PR, the coverage report should be generated without any issues, so please use this for future development :)

Copy link
Owner

Choose a reason for hiding this comment

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

Haven't seen a makefile in ages 😳

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 decided to use Make because it comes pre-installed on Linux systems and can function as a simple task runner.
In addition to Makefiles, there are several other well-known task runner tools. For Python, I've used tools like Taskipy before.

https://github.com/taskipy/taskipy

However, I think Make isn't installed by default on Windows systems, so if you think it's better to switch to a Python-based task runner, that's totally fine with me!

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, make is fine with me. I was more saying that because I haven't worked with C/C++ in ages and that makefile reminded me of pain🤣

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Haha, I totally get it now 😂

src/api.py Show resolved Hide resolved
src/server.py Show resolved Hide resolved
@ryansurf
Copy link
Owner

ryansurf commented May 29, 2024

Awww yeahh, solid work.

So from what I understand, to generate the coverage report as a comment on a PR I'll start developing in branches, submit a PR and then we'll see the pytest coverage report?

It would be cool to have this done on all PRs, but I suppose it makes sense that it cant 😔

You work very fast, thank you for the PR!

Edit: I see how it works now, whenever I merge a PR is generates the report. Seems like its working perfectly

@ryansurf ryansurf merged commit 440acf0 into ryansurf:main May 29, 2024
2 checks passed
@K-dash K-dash deleted the feature/introduce-ruff-and-test-runner branch May 30, 2024 01:19
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