Add Pylint to Workflow #373

freemansw1 · 2023-11-29T22:12:45Z

This is testing a theory, and is entirely copied from #278 . Per the GitHub documentation (https://docs.github.com/en/actions/security-guides/automatic-token-authentication#modifying-the-permissions-for-the-github_token), the Pylint runner will never work on Julia's fork (and in #278) as the maximum access for PRs from public forked repos is read. In theory, this, which copies over the changes from #278 to a branch within our repo, should work.

codecov · 2023-11-29T22:13:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f1d3a4e) 56.91% compared to head (fe1bad8) 59.80%.
Report is 5 commits behind head on RC_v1.5.x.

Additional details and impacted files

@@              Coverage Diff              @@
##           RC_v1.5.x     #373      +/-   ##
=============================================
+ Coverage      56.91%   59.80%   +2.88%     
=============================================
  Files             20       20              
  Lines           3440     3769     +329     
=============================================
+ Hits            1958     2254     +296     
- Misses          1482     1515      +33

Flag	Coverage Δ
unittests	`59.80% <ø> (+2.88%)`	⬆️

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.

github-actions · 2023-11-29T22:16:19Z

Linting results by Pylint:

Your code has been rated at 8.72/10 (previous run: 8.72/10, +0.00)
_{The linting score is an indicator that reflects how well your code version follows Pylint’s coding standards and quality metrics with respect to the RC_v1.5.x branch.

A decrease usually indicates your new code does not fully meet style guidelines or has potential errors.}

freemansw1 · 2023-11-29T22:19:33Z

OK Theory confirmed. @fziegner you should have write access to this branch, so feel free to edit/etc. probably good for you to get a commit in there to make sure your contributions are in the record.

freemansw1 · 2023-11-29T22:28:32Z

Probably better for this action to pull the merging branch, rather than main. We tend to do a lot of work in the RC branches.

w-k-jones

Looks good, my only specific comments are regarding how we install tobac

w-k-jones · 2023-11-30T11:39:57Z

.github/workflows/pylint.yml

+          pip install .
+          pip install --upgrade pylint


Would it be better to use conda/mamba to create the environment, consistent with other actions, rather than pip?

I had some trouble getting pylint to work with the conda environment but I'll take another look.

Ah fair, if it causes more work then it's probably not worth changing it. Seems to work fine for me via conda locally as long as it isn't using python 3.12

Also good to check that we can install tobac with pip!

Ok, it's working now. I just forgot to specify the shell in which pylint is supposed to run.

w-k-jones · 2023-11-30T11:40:34Z

.github/workflows/pylint.yml

+      - name: Set up Python
+        uses: actions/setup-python@v3
+        with:
+          python-version: '3.9'


Is there a need to use python 3.9? pylint doesn't seem to yet work with 3.12, but 3.11 works fine

I think we can change it to 3.11

github-actions · 2023-11-30T17:55:29Z

Linting results by Pylint:

Your code has been rated at 8.71/10 (previous run: 8.71/10, +0.00)
_{The linting score is an indicator that reflects how well your code version follows Pylint’s coding standards and quality metrics with respect to the RC_v1.5.x branch.

A decrease usually indicates your new code does not fully meet style guidelines or has potential errors.}

JuliaKukulies

Excellent, thanks for your help with this @freemansw1!

github-actions · 2023-11-30T19:37:52Z

Linting results by Pylint:

Your code has been rated at 8.72/10 (previous run: 8.72/10, +0.00)
_{The linting score is an indicator that reflects how well your code version follows Pylint’s coding standards and quality metrics with respect to the RC_v1.5.x branch.

A decrease usually indicates your new code does not fully meet style guidelines or has potential errors.}

fziegner

Thank you @freemansw1 for looking into the permission issue. Should be good to go now.

freemansw1 · 2023-12-01T17:36:19Z

.github/workflows/pylint.yml

+          echo "SHA=$(git rev-parse "$GITHUB_SHA")" >> $GITHUB_OUTPUT
+        id: git
+
+      - name: Get RC branch name


It would probably be better to grab the merging PR branch name. Let me look into that and see if that's possible

github-actions · 2023-12-01T17:41:48Z

Linting results by Pylint:

Your code has been rated at 8.72/10 (previous run: 8.72/10, +0.00)
_{The linting score is an indicator that reflects how well your code version follows Pylint’s coding standards and quality metrics with respect to the branch.

A decrease usually indicates your new code does not fully meet style guidelines or has potential errors.}

freemansw1 · 2023-12-01T17:43:07Z

Ok, I think that figures it out. @fziegner @fsenf @w-k-jones @JuliaKukulies let me know if you're still happy to merge.

w-k-jones · 2023-12-01T17:43:42Z

Is it possible to change the commenting action to make it update the origin comment (in the same manner as the coverage action) rather than post a new comment for each change? If it's not a quick change then we should go ahead and merge

freemansw1 · 2023-12-01T17:44:50Z

Is it possible to change the commenting action to make it update the origin comment (in the same manner as the coverage action) rather than post a new comment for each change? If it's not a quick change then we should go ahead and merge

That's an excellent point; this could get really annoying on frequently updated PRs

freemansw1 · 2023-12-04T15:18:59Z

Is it possible to change the commenting action to make it update the origin comment (in the same manner as the coverage action) rather than post a new comment for each change? If it's not a quick change then we should go ahead and merge

I'm going to suggest that this is a blocking issue given the (high) potential for several comments given the way we use PRs. Given the (potential) complexity there, I'm inclined to push this PR back to 1.5.3 (or between releases, as we have done before for repo enhancements). @fsenf @fziegner thoughts? Other than #376 (which is done in my eyes) this is the last thing to resolve before release.

freemansw1 · 2023-12-04T15:20:27Z

Amazing, proven wrong within a minute of commenting. Well done, @fziegner!

fziegner · 2023-12-04T15:23:25Z

Wow, that was incredible timing. The first comment by the linter in this PR should be edited now.

w-k-jones · 2023-12-04T15:24:10Z

Very nice work @fziegner , the first linter comment is showing as being edited 1 minute ago for me

freemansw1 · 2023-12-04T17:33:37Z

I can't approve my own PR, so this is me approving post edits. @fziegner fantastic work. Are you happy for us to merge?

fziegner · 2023-12-04T17:47:43Z

Yes @freemansw1, go ahead.

w-k-jones · 2023-12-04T19:10:27Z

Annoyingly the pylint workflow is still failing due to permissions when creating a PR from a forked repo. Looking at the documentation here it seems we need to use pull_request_target rather than pull_request to enable write permissions. I've made this change in #378 , but I think that for this trigger to work it needs to be part of the branch being merged into (i.e. in the main repository) rather than the merging branch

JuliaKukulies · 2023-12-06T08:49:01Z

Excellent work @fziegner!

Annoyingly the pylint workflow is still failing due to permissions when creating a PR from a forked repo. Looking at the documentation here it seems we need to use pull_request_target rather than pull_request to enable write permissions. I've made this change in #378 , but I think that for this trigger to work it needs to be part of the branch being merged into (i.e. in the main repository) rather than the merging branch

So that would just mean that we update pylint.yml one more time and change to/add pull_request_target in the RC branch which will then be merged to main with the release or what do you think?

w-k-jones · 2023-12-06T12:16:59Z

So that would just mean that we update pylint.yml one more time and change to/add pull_request_target in the RC branch which will then be merged to main with the release or what do you think?

I've committed the change directly to the RC_v1.5.x branch and it's now working correctly

JuliaKukulies · 2023-12-08T13:11:45Z

So that would just mean that we update pylint.yml one more time and change to/add pull_request_target in the RC branch which will then be merged to main with the release or what do you think?

I've committed the change directly to the RC_v1.5.x branch and it's now working correctly

Perfect, thanks!

test pylint action

1083ada

freemansw1 mentioned this pull request Nov 29, 2023

Introducing pylint in our workflow #278

Closed

11 tasks

freemansw1 requested review from w-k-jones, fsenf, JuliaKukulies and fziegner November 29, 2023 22:16

freemansw1 added this to the Version 1.5.2 milestone Nov 29, 2023

freemansw1 added the enhancement Addition of new features, or improved functionality of existing features label Nov 29, 2023

w-k-jones approved these changes Nov 30, 2023

View reviewed changes

changed comparison from main to RC branch

383e9f7

JuliaKukulies approved these changes Nov 30, 2023

View reviewed changes

changed environment to mamba

26768f5

fziegner approved these changes Nov 30, 2023

View reviewed changes

freemansw1 commented Dec 1, 2023

View reviewed changes

update to use target branch name

b0f7f60

w-k-jones linked an issue Dec 2, 2023 that may be closed by this pull request

Time to add pull request linting? #273

Closed

changed linting message creation to updating exisiting comment

fe1bad8

freemansw1 changed the title ~~Test PR for internal pylint branch~~ Add Pylint to Workflow Dec 4, 2023

freemansw1 merged commit 7ded724 into RC_v1.5.x Dec 4, 2023
38 checks passed

freemansw1 mentioned this pull request Dec 4, 2023

Time to add pull request linting? #273

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Pylint to Workflow #373

Add Pylint to Workflow #373

freemansw1 commented Nov 29, 2023

codecov bot commented Nov 29, 2023 •

edited

Loading

github-actions bot commented Nov 29, 2023 •

edited

Loading

freemansw1 commented Nov 29, 2023

freemansw1 commented Nov 29, 2023

w-k-jones left a comment

w-k-jones Nov 30, 2023

fziegner Nov 30, 2023

w-k-jones Nov 30, 2023

fziegner Nov 30, 2023

w-k-jones Nov 30, 2023

JuliaKukulies Nov 30, 2023

github-actions bot commented Nov 30, 2023

JuliaKukulies left a comment

github-actions bot commented Nov 30, 2023

fziegner left a comment

freemansw1 Dec 1, 2023

github-actions bot commented Dec 1, 2023

freemansw1 commented Dec 1, 2023

w-k-jones commented Dec 1, 2023 •

edited

Loading

freemansw1 commented Dec 1, 2023

freemansw1 commented Dec 4, 2023

freemansw1 commented Dec 4, 2023

fziegner commented Dec 4, 2023

w-k-jones commented Dec 4, 2023

freemansw1 commented Dec 4, 2023

fziegner commented Dec 4, 2023

w-k-jones commented Dec 4, 2023

JuliaKukulies commented Dec 6, 2023

w-k-jones commented Dec 6, 2023

JuliaKukulies commented Dec 8, 2023

Add Pylint to Workflow #373

Add Pylint to Workflow #373

Conversation

freemansw1 commented Nov 29, 2023

codecov bot commented Nov 29, 2023 • edited Loading

Codecov Report

github-actions bot commented Nov 29, 2023 • edited Loading

Linting results by Pylint:

freemansw1 commented Nov 29, 2023

freemansw1 commented Nov 29, 2023

w-k-jones left a comment

Choose a reason for hiding this comment

w-k-jones Nov 30, 2023

Choose a reason for hiding this comment

fziegner Nov 30, 2023

Choose a reason for hiding this comment

w-k-jones Nov 30, 2023

Choose a reason for hiding this comment

fziegner Nov 30, 2023

Choose a reason for hiding this comment

w-k-jones Nov 30, 2023

Choose a reason for hiding this comment

JuliaKukulies Nov 30, 2023

Choose a reason for hiding this comment

github-actions bot commented Nov 30, 2023

Linting results by Pylint:

JuliaKukulies left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 30, 2023

Linting results by Pylint:

fziegner left a comment

Choose a reason for hiding this comment

freemansw1 Dec 1, 2023

Choose a reason for hiding this comment

github-actions bot commented Dec 1, 2023

Linting results by Pylint:

freemansw1 commented Dec 1, 2023

w-k-jones commented Dec 1, 2023 • edited Loading

freemansw1 commented Dec 1, 2023

freemansw1 commented Dec 4, 2023

freemansw1 commented Dec 4, 2023

fziegner commented Dec 4, 2023

w-k-jones commented Dec 4, 2023

freemansw1 commented Dec 4, 2023

fziegner commented Dec 4, 2023

w-k-jones commented Dec 4, 2023

JuliaKukulies commented Dec 6, 2023

w-k-jones commented Dec 6, 2023

JuliaKukulies commented Dec 8, 2023

codecov bot commented Nov 29, 2023 •

edited

Loading

github-actions bot commented Nov 29, 2023 •

edited

Loading

w-k-jones commented Dec 1, 2023 •

edited

Loading