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

-install with --keep-* options #188

Closed
wants to merge 9 commits into from

Conversation

igor-sb
Copy link

@igor-sb igor-sb commented Jan 25, 2024

Hi, I would like to contribute a PR that resolves this issue:

#150 : --install with other (--keep-*) options

  • I added another argument to the install() function, which concatenates those options into the .git/config
  • I manually tested this functionality myself.
  • I bumped the tag to 0.6.2.
  • Did not add any new unit tests.

Let me know what you think. Thank you!

JasonJoosteCSIRO and others added 5 commits May 5, 2023 21:23
* Add command line argument keep-id, which maintiains randomly generated cell ids. Otherwise cell ids are assigned incrementally (after the removal of cells), which should keep them consistent across runs in version control

* Modify test_cell and test_exception in test_keep_output_tags.py to use the new strip_output signature

* Fix failed test_end_to_end_nbstripout with test_max_size by passing --keep-id for keeping the existing ids

* Add tests for notebooks with and without the --keep-id flag. A new extension expected_id was added for expected output with ordered ids

* Modify the readme to include the --include-id flag

* Add keyword arguments for None inputs in test_keep_output_tags.py

* Rename expected output files to make desired sequential ids more explicit

Co-authored-by: Florian Rathgeber <[email protected]>
* Clarify that extra keys may _only_ specify notebook and cell metadata to be stripped.
* Update metadata stripped by default.

Addresses #187
README.rst Outdated
@@ -34,7 +34,7 @@ Based on https://gist.github.com/minrk/6176788.
Python 3 only
=============

As of version 0.6.1, nbstripout supports Python 3 *only*. If you need to use
As of version 0.6.2, nbstripout supports Python 3 *only*. If you need to use
Copy link
Owner

Choose a reason for hiding this comment

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

Please revert this change.

Copy link
Author

Choose a reason for hiding this comment

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

Reverted.

Comment on lines 221 to 222
if keep_args:
filepath = filepath + ' '.join(keep_args)
Copy link
Owner

Choose a reason for hiding this comment

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

I think I'd rather configure these options via the git config directly i.e. if you want to use keep_output with the git filter, you set git config --type bool filter.nbstripout.keepOutput true and then we read that value to provide the default for keep_output.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense. Here's my take on this:

  • I am passing extra_flags to install() and uninstall() that keep track of the three flags keep_output, keep_id and keep_count, so they can be cleaned up.
  • Since the underscores don't work for git config names, I am converting to camelCase
  • If these flags are set in the cmd args, that will override whatever git config values are
  • If the flags are not passed as the cmd args, do not exist in git config (e.g. running nbstripout without install), then they will be False

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 for my lack of follow up: I think I'd prefer addressing this differently, see #189. I have some preliminary implementation for this which I've pushed to a branch, PTAL :)

Comment on lines +159 to +165
def snake_to_camel_case(string):
"""Converts snake_case string to camelCase."""
if '_' not in string:
return string
string_parts = string.split('_')
camel_parts = [part[0].upper() + part[1:] for part in string_parts[1:] if part != '']
return string_parts[0] + ''.join(camel_parts)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
def snake_to_camel_case(string):
"""Converts snake_case string to camelCase."""
if '_' not in string:
return string
string_parts = string.split('_')
camel_parts = [part[0].upper() + part[1:] for part in string_parts[1:] if part != '']
return string_parts[0] + ''.join(camel_parts)
def to_lower_camel_case(s):
"""Converts snake_case string to lowerCamelCase."""
head, *tail = s.split('_')
return head + ''.join(p.title() for p in tail)

@igor-sb igor-sb closed this by deleting the head repository Nov 3, 2024
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.

3 participants