ryansurf / cli-surf

Get surf and ocean data from the command line interface
7 stars 10 forks source link

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

Closed K-dash closed 1 month ago

K-dash commented 2 months ago

@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:

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 commented 2 months ago

@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.

https://github.com/MishaKav/pytest-coverage-comment/issues/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 commented 2 months ago

@ryansurf https://github.com/ryansurf/cli-surf/pull/26#issuecomment-2136542237 <- 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 :)

ryansurf commented 1 month ago

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 :pensive:

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