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

Introducing pylint in our workflow #278

Closed

Conversation

JuliaKukulies
Copy link
Member

This PR addresses #273 and adds a pylint check in the github actions. pylint is run on PRs to the RC_* branch and the resulting score is posted as a comment to the respective PR. The full length output of the linting can be seen in the details of the action.

In addition, I fixed the unused import issues in our modules. The resulting score for the entire tobac package is currently 8.78/10 (if conventions are disabled ) and 6.72/10 if not -- not so bad actually.

What I did not manage to implement, though, was to compare the score of PRs to previous scores (i.e. the score of the current main. This would be valuable because it would tell contributors if the score has deteriorated. Given that the python environment is re-installed with every action, the previous scores are not stored. I tried both caching of the python environment as well as running pylint on main within the steps of an action, but both did not seem to work. Any ideas on how to achieve this?

  • Have you followed our guidelines in CONTRIBUTING.md?
  • Have you self-reviewed your code and corrected any misspellings?
  • Have you written documentation that is easy to understand?
  • Have you written descriptive commit messages?
  • Have you added NumPy docstrings for newly added functions? N/A
  • Have you formatted your code using black? N/A
  • If you have introduced a new functionality, have you added adequate unit tests? N/A
  • Have all tests passed in your local clone?
  • If you have introduced a new functionality, have you added an example notebook? N/A
  • Have you kept your pull request small and limited so that it is easy to review?
  • Have the newest changes from this branch been merged?

@JuliaKukulies JuliaKukulies added the workflow Improvements and updates to the repository and CI label May 10, 2023
@JuliaKukulies
Copy link
Member Author

OK, I figured this did not work out as in my fork due to permissions granted to the GITHUB_TOKEN. Is it reasonable to adjust these, in order to post the score as comment or are you aware of another way?

@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

Attention: 42 lines in your changes are missing coverage. Please review.

Comparison is base (17392d0) 56.79% compared to head (db34106) 56.89%.
Report is 145 commits behind head on RC_v1.5.x.

Files Patch % Lines
tobac/utils/internal/basic.py 80.73% 21 Missing ⚠️
tobac/utils/bulk_statistics.py 86.88% 8 Missing ⚠️
tobac/utils/internal/iris_utils.py 88.00% 6 Missing ⚠️
tobac/utils/internal/xarray_utils.py 71.42% 4 Missing ⚠️
tobac/utils/decorators.py 96.51% 3 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##           RC_v1.5.x     #278      +/-   ##
=============================================
+ Coverage      56.79%   56.89%   +0.09%     
=============================================
  Files             20       20              
  Lines           3435     3438       +3     
=============================================
+ Hits            1951     1956       +5     
+ Misses          1484     1482       -2     
Flag Coverage Δ
unittests 56.89% <88.23%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@freemansw1
Copy link
Member

OK, I figured this did not work out as in my fork due to permissions granted to the GITHUB_TOKEN. Is it reasonable to adjust these, in order to post the score as comment or are you aware of another way?

I'm happy for us to adjust these. For adding the coverage bot, I ended up doing my work in a branch of the main repo rather than on a personal fork; that might be helpful here?

@JuliaKukulies
Copy link
Member Author

That's a good point @freemansw1, will do!

@freemansw1
Copy link
Member

@JuliaKukulies Do you want this in v1.5.0, or are you happy to wait for a later release?

@JuliaKukulies
Copy link
Member Author

@freemansw1 later release, thanks for asking! This is really not urgent and that will give me some time to figure out how to do that the smartest way

@JuliaKukulies JuliaKukulies added this to the Version 1.6 milestone May 22, 2023
@w-k-jones
Copy link
Member

Like #258 we can introduce workflow actions independently of releases, correct?

@JuliaKukulies
Copy link
Member Author

Like #258 we can introduce workflow actions independently of releases, correct?

That makes sense, yes!

@JuliaKukulies JuliaKukulies removed this from the Version 1.6 milestone May 22, 2023
@freemansw1 freemansw1 marked this pull request as draft May 22, 2023 17:52
@JuliaKukulies JuliaKukulies changed the base branch from RC_v1.5.0 to RC_v1.5.x July 14, 2023 20:15
@fsenf fsenf added this to the Version 1.5.2 milestone Nov 23, 2023
@fsenf
Copy link
Member

fsenf commented Nov 23, 2023

Hej all, this is also nearly ready! @fziegner has a nice working version there.

Workflow: Fabian is updating his PR onto Julias fork. After Julia has accepted this PR should be ready to be reviewed.

@fsenf fsenf mentioned this pull request Nov 23, 2023
8 tasks
@JuliaKukulies JuliaKukulies marked this pull request as ready for review November 28, 2023 14:20
@JuliaKukulies
Copy link
Member Author

@fziegner do you know why the linting fails here?

@fziegner
Copy link
Collaborator

From the comments earlier this year it looks like it was already changed but can you check if the Workflow has Write access? (https://stackoverflow.com/a/76199390)

@freemansw1
Copy link
Member

From the comments earlier this year it looks like it was already changed but can you check if the Workflow has Write access? (https://stackoverflow.com/a/76199390)

This was indeed changed, and I can confirm "Workflows have read and write permissions in the repository for all scopes."

@freemansw1 freemansw1 mentioned this pull request Nov 29, 2023
11 tasks
@freemansw1
Copy link
Member

OK, I think I figured out the issue. See my discussion in #373

@freemansw1
Copy link
Member

We can either close this PR or modify it to just be the enhanced linting. Your choice, @fziegner @JuliaKukulies

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
workflow Improvements and updates to the repository and CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants