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

[MRG] Add lint-action workflow #1711

Merged
merged 1 commit into from Oct 12, 2022
Merged

Conversation

mrbean-bremen
Copy link
Member

@mrbean-bremen mrbean-bremen commented Oct 9, 2022

@codecov
Copy link

codecov bot commented Oct 9, 2022

Codecov Report

Merging #1711 (08fc246) into master (16fd77d) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1711      +/-   ##
==========================================
+ Coverage   97.58%   97.60%   +0.01%     
==========================================
  Files          66       66              
  Lines       10744    10744              
==========================================
+ Hits        10485    10487       +2     
+ Misses        259      257       -2     
Impacted Files Coverage Δ
pydicom/filebase.py 99.19% <0.00%> (+1.61%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mrbean-bremen mrbean-bremen marked this pull request as draft October 9, 2022 16:38
@mrbean-bremen
Copy link
Member Author

mrbean-bremen commented Oct 9, 2022

@darcymason - I found the action reviewdog/action-flake8, which does annotations. Please have a look at the diff, if this is what you had in mind. We could use this instead of Pep8Speaks. We already do flake8 checks, have to check if we can use the settings from that check, and replace that also.
There is is also action-black if we want this.

@darcymason
Copy link
Member

Please have a look at the diff, if this is what you had in mind. We could use this instead of Pep8Speaks. We already do flake8 checks, have to check if we can use the settings from that check, and replace that also.
There is is also action-black if we want this.

Yeah, this looks pretty good. I think we might as well use action-black as well, might as well try to standardize the formatting as code comes in.

@mrbean-bremen mrbean-bremen force-pushed the lint-action branch 2 times, most recently from f64ebc0 to 8c09a7d Compare October 11, 2022 17:44
@mrbean-bremen
Copy link
Member Author

@darcymason: Not sure about black - I enabled it and configured it to check all files (instead of the diff only), and it shows a few things in example code, but doesn't really say what exactly is wrong (you may check the curent diff). Also, there seem to be some general limits for this kind of annotations, so not all warnings are shown:

Error: reviewdog: Too many results (annotations) in diff.
You may miss some annotations due to GitHub limitation for annotation created by logging command.
Please check GitHub Actions log console to see all results.

Limitation:
- 10 warning annotations and 10 error annotations per step
- 50 annotations per job (sum of annotations from all the steps)
- 50 annotations per run (separate from the job annotations, these annotations aren't created by users)

I will change it to only check the diff, maybe it is still helpful... for the time being I would leave it at warnings only (e.g. no failed build on findings).

flake8 is now configured as before: first for errors, which will fail the build (as you had seen in a previous run), and afterwards for style warnings, which will not fail the build. Bothe are run on the diff, but I can see that there are a lot of warnings (but no errors) outside the diffs:

reviewdog: No results found for "flake8". 7296 results found outside diff.

I may leave it as is for the time being, and we can later check what to change... what do you think?

@mrbean-bremen mrbean-bremen marked this pull request as ready for review October 11, 2022 19:06
@darcymason
Copy link
Member

I will change it to only check the diff, maybe it is still helpful.

Agreed.

flake8 is now configured as before: first for errors, which will fail the build (as you had seen in a previous run), and afterwards for style warnings, which will not fail the build

I agree with this also.

I may leave it as is for the time being, and we can later check what to change... what do you think?

I think "as is" still includes black on all, based on your comments? If so, I prefer the only on the diff option.

@mrbean-bremen
Copy link
Member Author

I think "as is" still includes black on all, based on your comments? If so, I prefer the only on the diff option.

No, I left that only in so you can see the diff. Will change this now...

- use same flake8 arguments as before
@mrbean-bremen mrbean-bremen changed the title [WIP] Add lint-action workflow [MRG] Add lint-action workflow Oct 11, 2022
@mrbean-bremen
Copy link
Member Author

I actually don't remember where Pep8Speaks is configured, in case we want to remove it...

@mrbean-bremen
Copy link
Member Author

I actually don't remember where Pep8Speaks is configured, in case we want to remove it...

Found it, under Settings / Applications. I would suggest to leave it in for a time even if we merge this, to see if it finds stuff that flake8 does not.

Copy link
Member

@darcymason darcymason left a comment

Choose a reason for hiding this comment

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

Looks good. Lets merge it in and se how things go...

@darcymason darcymason merged commit a8be738 into pydicom:master Oct 12, 2022
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