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: add --config cli argument #1081

Merged
merged 6 commits into from
May 24, 2021
Merged

Conversation

reiddraper
Copy link
Contributor

@reiddraper reiddraper commented Jun 10, 2020

This is useful for projects that want to provide their own proselintrc
which is stored in the repository for the project itself. Further, it's
useful for installations where putting a config-file outside of a
sandboxed directory is not possible.


Resolves #356. Resolves #1091.

@reiddraper
Copy link
Contributor Author

Looks like there's a few build failures related to different APIs on different Python versions. Before I fix each of them, is this a contribution you'd be interested in?

@carlsmedstad
Copy link
Contributor

Apart from needing a rebase on main and some clean up to pass the CI, this looks good to me.

Should @reiddraper continue this work? @Nytelife26 @suchow

@Nytelife26
Copy link
Member

Thanks for bringing this to my attention, @carlsmedstad. I'll get to work on bringing it up to date and merging now, since it is a useful feature. We could also put automatically detecting proselintrc files up to the filesystem boundary as part of this PR, too, but that's another thing.

@Nytelife26 Nytelife26 added priority: medium Issues and PRs that should be resolved soon. status: wip Issues and PRs that are still a work in progress. type: feat Issues and PRs related to new features. version: minor Issues and PRs with new features belonging to the next minor release. labels May 23, 2021
@carlsmedstad
Copy link
Contributor

carlsmedstad commented May 23, 2021

Great, detecting project specific config files seems like something that would be useful after this has been merged. This would resolve #1112 and #702.

I'm a proponent of the pattern of having three levels of configuration: system, user and project scope.

@Nytelife26
Copy link
Member

Nytelife26 commented May 23, 2021 via email

This is useful for projects that want to provide their own proselintrc
which is stored in the repository for the project itself. Further, it's
useful for installations where putting a config-file outside of a
sandboxed directory is not possible.
@codecov
Copy link

codecov bot commented May 23, 2021

Codecov Report

Merging #1081 (905ead6) into main (50ac6b9) will increase coverage by 0.98%.
The diff coverage is 95.65%.

❗ Current head 905ead6 differs from pull request most recent head b825a57. Consider uploading reports for the commit b825a57 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1081      +/-   ##
==========================================
+ Coverage   89.20%   90.19%   +0.98%     
==========================================
  Files          83       83              
  Lines        1214     1203      -11     
==========================================
+ Hits         1083     1085       +2     
+ Misses        131      118      -13     
Flag Coverage Δ
macos-latest 90.19% <95.65%> (+0.98%) ⬆️
py3.6 89.22% <95.65%> (+1.06%) ⬆️
py3.7 89.22% <95.65%> (+1.06%) ⬆️
py3.8 90.19% <95.65%> (+0.98%) ⬆️
py3.9 90.19% <95.65%> (+0.98%) ⬆️
pypypy3 89.22% <95.65%> (+1.06%) ⬆️
ubuntu-latest 90.19% <95.65%> (+0.98%) ⬆️
windows-latest 90.19% <95.65%> (+0.98%) ⬆️

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

Impacted Files Coverage Δ
proselint/command_line.py 44.23% <66.66%> (+0.54%) ⬆️
proselint/tools.py 77.56% <100.00%> (+3.74%) ⬆️

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 50ac6b9...b825a57. Read the comment docs.

@carlsmedstad
Copy link
Contributor

Definitely, I could make a PR tomorrow if you'd like?

@Nytelife26 Nytelife26 self-requested a review May 23, 2021 14:20
@Nytelife26 Nytelife26 added status: awaiting-tests Issues and PRs that require tests to proceed. and removed status: wip Issues and PRs that are still a work in progress. labels May 23, 2021
@suchow
Copy link
Member

suchow commented May 23, 2021

@Nytelife26 The CI run is failing here for one substantive reason (coverage) and one transition-to-Github-actions reason. In particular, something is breaking during the Danger action.

@Nytelife26
Copy link
Member

Nytelife26 commented May 24, 2021 via email

@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 May 24, 2021
@Nytelife26 Nytelife26 requested a review from suchow May 24, 2021 09:31
@Nytelife26
Copy link
Member

cc: @suchow

tests/test_config_flag.py Outdated Show resolved Hide resolved
@Nytelife26 Nytelife26 requested a review from suchow May 24, 2021 18:37
Copy link
Member

@suchow suchow left a comment

Choose a reason for hiding this comment

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

The new test looks good, just need to import subprocess up top and I think it should be good to go!

@Nytelife26 Nytelife26 changed the title Add --config flag to override configuration file feat: add --config cli argument May 24, 2021
@Nytelife26 Nytelife26 merged commit b4bca54 into amperser:main May 24, 2021
@benknoble
Copy link

This doesn't resolve #1091, which is about individual checks versus categories.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium Issues and PRs that should be resolved soon. 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.

Allow disabling individual checks Config files are loaded only from a central location
5 participants