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

feat: implement project-wide proselintrc #1173

Merged

Conversation

carlsmedstad
Copy link
Contributor

@carlsmedstad carlsmedstad commented Jun 6, 2021

Hey again 👋

This PR makes proselint pick up .proselintrc file in the current working directory, i.e. project specific config files. Also included some refactoring and error reporting.

Resolves #1112.

Make the option loading logic more easy to follow:
* Centralize all paths looked for in the function by removing the
  fallback argument.
* Cleary read one set of system options and one set of user options and
  then merge these two sets.
* Split out dict merging to new function.
This makes proselint read a .proselintrc file in the current working
directory if such a file exists. This file will take precedence over
the users configuration but not over a file provided with the --config
option.
@Nytelife26
Copy link
Member

Thank you for the pull request! I'll triage & review now.

@Nytelife26 Nytelife26 added cat: maintenance Issues and PRs related to the maintenance of a module. priority: low Issues and PRs that should be resolved, but can be postponed. status: review-ready PRs that are ready for author review. type: feat Issues and PRs related to new features. version: minor Issues and PRs with new features belonging to the next minor release. labels Jun 7, 2021
@codecov
Copy link

codecov bot commented Jun 7, 2021

Codecov Report

Merging #1173 (eb221a7) into main (ae2161b) will increase coverage by 4.51%.
The diff coverage is 94.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1173      +/-   ##
==========================================
+ Coverage   90.19%   94.71%   +4.51%     
==========================================
  Files          83       83              
  Lines        1203     1210       +7     
==========================================
+ Hits         1085     1146      +61     
+ Misses        118       64      -54     
Flag Coverage Δ
macos-latest 94.71% <94.44%> (+4.51%) ⬆️
py3.6 94.19% <94.44%> (+4.96%) ⬆️
py3.7 94.19% <94.44%> (+4.96%) ⬆️
py3.8 94.71% <94.44%> (+4.51%) ⬆️
py3.9 94.71% <94.44%> (+4.51%) ⬆️
pypypy3 94.19% <94.44%> (+4.96%) ⬆️
ubuntu-latest 94.71% <94.44%> (+4.51%) ⬆️
windows-latest 94.71% <94.44%> (+4.51%) ⬆️

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

Impacted Files Coverage Δ
proselint/command_line.py 73.33% <80.00%> (+29.10%) ⬆️
proselint/tools.py 86.98% <100.00%> (+9.42%) ⬆️

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 ae2161b...eb221a7. Read the comment docs.

@Nytelife26 Nytelife26 added status: awaiting-tests Issues and PRs that require tests to proceed. and removed status: review-ready PRs that are ready for author review. labels Jun 7, 2021
@Nytelife26
Copy link
Member

@carlsmedstad can you fix the coverage issues by adjusting the relevant unit tests for your changes?

@carlsmedstad
Copy link
Contributor Author

carlsmedstad commented Jun 7, 2021

@carlsmedstad can you fix the coverage issues by adjusting the relevant unit tests for your changes?

I extended one test., hopefully this increases the coverage enough. I'm not sure how to run the codecov job locally.

@suchow
Copy link
Member

suchow commented Jun 7, 2021

@Nytelife26 We’re getting build failures here and in some other PRs for reasons unrelated to the PR itself — Codecov is failing to upload results.

@Nytelife26
Copy link
Member

Codecov is failing to upload results.

I noticed this in another pull request. I am not entirely sure if Codecov was down briefly or not, but I'll try rerunning now a few hours later to see if it has been resolved yet.

@Nytelife26
Copy link
Member

Nytelife26 commented Jun 7, 2021

hopefully this increases the coverage enough. I'm not sure how to run the codecov job locally.

You cannot, unfortunately. The best way to approach this is to check within Codecov's UI for the uncovered lines and add testing for them.

It appears that for this PR most of the coverage loss stems from proselint/command_line.py.
To resolve the coverage issues, you need to:

  • Add testing that triggers the sys.exit(2) statement
  • Add testing for if the configuration file is not a file

I will adjust the Codecov settings to set a threshold for coverage loss until then.

"non_existing_file"],
stderr=subprocess.PIPE, encoding='utf-8')
assert output.returncode == 2
assert 'FileNotFoundError' in output.stderr
Copy link
Contributor Author

@carlsmedstad carlsmedstad Jun 7, 2021

Choose a reason for hiding this comment

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

It appears that for this PR most of the coverage loss stems from proselint/command_line.py.
To resolve the coverage issues, you need to:

  • Add testing that triggers the sys.exit(2) statement
  • Add testing for if the configuration file is not a file

I will adjust the Codecov settings to set a threshold for coverage loss until then.

That was exactly what I though I did. Either the test is not working, or codecov is not picking this up.

Copy link
Member

Choose a reason for hiding this comment

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

Apologies, I didn't see this until just now. I'll investigate later tonight.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries! Feel free to ping me if you need me for anything.

@Nytelife26
Copy link
Member

Nytelife26 commented Jul 3, 2021

@carlsmedstad As it happens, you were right - there was an issue with the way we were testing it. Python coverage does not pick up subprocess calls, so switching to Click's unit testing fixed it. Thanks for the PR - we're now ready for review.

cc: @suchow

@Nytelife26 Nytelife26 added status: review-ready PRs that are ready for author review. and removed status: awaiting-tests Issues and PRs that require tests to proceed. labels Jul 3, 2021
@Nytelife26 Nytelife26 changed the title Implement project config file feat: implement project-wide proselintrc Jul 4, 2021
@Nytelife26 Nytelife26 merged commit 81a3943 into amperser:main Jul 4, 2021
@carlsmedstad carlsmedstad deleted the implement-project-config-file branch July 10, 2021 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat: maintenance Issues and PRs related to the maintenance of a module. priority: low Issues and PRs that should be resolved, but can be postponed. status: review-ready PRs that are ready for author review. type: feat Issues and PRs related to new features. version: minor Issues and PRs with new features belonging to the next minor release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.proselintrc is not picked up
3 participants