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

Add a --system argument for system-wide installation #149

Merged
merged 4 commits into from
Apr 24, 2021
Merged

Add a --system argument for system-wide installation #149

merged 4 commits into from
Apr 24, 2021

Conversation

PLPeeters
Copy link
Contributor

This PR adds a --system argument for system-wide installation.

Since there doesn't seem to be a simple way to figure out the $(prefix) variable of the Git installation, I had to resort to a kind of hacky way to figure that one out in order to put the system-wide gitattributes file in the correct place (see the _get_system_gitconfig_folder() function).

I also fixed the path of the gitattributes file in the documentation when using the --global argument, which was incorrectly documented as being .git/info/attributes instead of ~/.config/git/attributes.

I also took the liberty of adding a few newlines to improve readability.

@PLPeeters
Copy link
Contributor Author

I also just went ahead and added a fix and a test for the --status command which was apparently broken, along with some unification in the treatment of shell output (setting text=True everywhere and removing calls to .decode() that are no longer necessary with this). I can do a separate PR for that if you prefer, but since it intersects with my changes it was just easier for me to include it in here.

Copy link
Owner

@kynan kynan left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! That's a useful feature.

Thanks also for the other fixes / cleanup and for adding a test.

Just a minor nit, otherwise I think this is good to go!

README.rst Outdated Show resolved Hide resolved
nbstripout/_nbstripout.py Show resolved Hide resolved
nbstripout/_nbstripout.py Outdated Show resolved Hide resolved
@kynan kynan added type:enhancement state:waiting Waiting for response for reporter labels Apr 15, 2021
@kynan kynan added this to the 0.4.0 milestone Apr 15, 2021
Copy link
Owner

@kynan kynan left a comment

Choose a reason for hiding this comment

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

Thanks, I think this is good to go now!

Copy link
Owner

@kynan kynan left a comment

Choose a reason for hiding this comment

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

Sorry, spoke too soon, this is not passing tests:

  1. looks like the text parameter to check_output is 3.7+ only. Can you use universal_newlines instead?
  2. we're getting lots of stray output: fatal: --local can only be used inside a git repository

@PLPeeters
Copy link
Contributor Author

@kynan Changed the text parameter as requested.

The stray output was due to the git config call to fetch the extra keys. I changed it to a simple git config when nbstripout is called without the --system or --global parameters, since that seemed to make the most sense.

I also fixed the Git test since it didn't run properly on my system even though it looks fine on Travis. Apparently the version of Git on macOS seems to insist on having a newline at the end of the attributes file (it threw an *.txt is not a valid attribute name: .git/info/attributes:1 error on all the Git calls in that test for me).

@PLPeeters PLPeeters requested a review from kynan April 22, 2021 08:14
Copy link
Owner

@kynan kynan left a comment

Choose a reason for hiding this comment

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

Thanks, excellent work, really appreciate your contribution :)

@kynan kynan merged commit 6329810 into kynan:master Apr 24, 2021
@kynan kynan added resolution:merged type:bug type:tests and removed state:waiting Waiting for response for reporter labels Apr 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants