Skip to content

Add an option to show violation shortlinks in the formatter output #2375

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

Merged
merged 11 commits into from
Apr 8, 2022

Conversation

notpushkin
Copy link
Contributor

@notpushkin notpushkin commented Mar 20, 2022

Checklist

  • I have double checked that there are no unrelated changes in this pull request (old patches, accidental config files, etc)
  • I have created at least one test case for the changes I have made
  • I have updated the documentation for the changes I have made
  • I have added my changes to the CHANGELOG.md

Related issues

Closes #2374

@notpushkin
Copy link
Contributor Author

@sobolevn I'm currently looking at how testing is done for the formatter. Should I just add a variation to the @parametrize() decorator in test_formatter_output.py and commit the generated snapshots?

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thank you! 👍

@notpushkin notpushkin requested a review from sobolevn March 26, 2022 09:59
@notpushkin
Copy link
Contributor Author

notpushkin commented Mar 26, 2022

How should we deal with test_configuration? I'm not sure if this option can be documented in the same way as violation options (I've documented it on the Formatter page though). Maybe just add an exception for it (if option_item is not in FORMATTING_OPTIONS: assert or, even better, just exclude it from option_listed)?

@notpushkin
Copy link
Contributor Author

notpushkin commented Mar 26, 2022

Also, any ideas how to get that last 1% of coverage for wemake_python_styleguide/formatter.py?

image

Apparently it has to do with this piece of code – I've taken it verbatim from BaseFormatter.handle/write. I can't come up with a case when line can be None but I'm not sure I can just remove this condition either.

@sobolevn
Copy link
Member

I think that line is always present 🤔

@notpushkin
Copy link
Contributor Author

notpushkin commented Apr 7, 2022

safety check is failing – Click needs to be updated. This is out of scope of this PR, though.

@sobolevn sobolevn closed this Apr 8, 2022
@sobolevn sobolevn reopened this Apr 8, 2022
@sobolevn
Copy link
Member

sobolevn commented Apr 8, 2022

Now it should be fine! 👍

@codecov
Copy link

codecov bot commented Apr 8, 2022

Codecov Report

Merging #2375 (12004a0) into master (58e0fe3) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #2375   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          120       120           
  Lines         6394      6408   +14     
  Branches      1442      1445    +3     
=========================================
+ Hits          6394      6408   +14     
Impacted Files Coverage Δ
wemake_python_styleguide/options/config.py 100.00% <ø> (ø)
wemake_python_styleguide/formatter.py 100.00% <100.00%> (ø)
wemake_python_styleguide/options/defaults.py 100.00% <100.00%> (ø)
wemake_python_styleguide/options/validation.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58e0fe3...12004a0. Read the comment docs.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thank you, great feature!

I also think that adding --violation-links-prefix= is a good idea.
So, users can change services:

  • --violation-links-prefix=https://ya.ru will result in https://ya.ru/WPS001
  • etc

If you are interested, you can send one more PR.

Bery big thanks for your work and financial support! 👍

@sobolevn sobolevn merged commit a6e32b9 into wemake-services:master Apr 8, 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.

Violation shortlinks in the formatter
2 participants