Support config files #169

janosh · 2022-10-01T19:01:18Z

Closes #166.

The code in this PR was adapted from PyCQA/autoflake#79. Big thank you and credit to @akeeman.

kynan

Thanks for your contribution! I don't like multiple changes conflated in the same PR, so please submit all your unrelated changes as separate PRs as requested.

nbstripout/_nbstripout.py

pre-commit

.github/workflows/tests.yml

CHANGELOG.rst

nbstripout/_nbstripout.py

kynan · 2022-10-02T10:19:10Z

nbstripout/_utils.py

+
+ # Traverse the file tree common to all files given as argument looking for
+ # a configuration file
+ config_path = os.path.commonpath([os.path.abspath(file) for file in args.files]) if args.files else os.getcwd()


Please use pathlib wherever possible.

@kynan Thanks for the quick review. I believe I addressed all your comments except this one. I'm not too familiar with the pathlib module. Just did a quick search and couldn't find sth equivalent to os.path.commonpath(). Could you be more specific here?

Yes, unfortunately there is no equivalent to commonpath in pathlib. I won't insist, so you can leave as-is :)

There's a whole interesting thread on S/O on commonpath -
https://stackoverflow.com/a/43613742

Though not nessacarily helpful - they do end up still using commonpath, and just wrapping it with Path.

nbstripout/_nbstripout.py

nbstripout/_utils.py

CHANGELOG.rst

kynan · 2022-10-02T10:31:55Z

Please ensure your changes are compatible with Python 3.6 onwards.

…n_file() in nbstripout/_utils.py

…rser.parse_args()

kynan

I think this approach is actually much better than what is implemented now. The approach is:

Read config files and turn their defined keys and values into "pseudo" command line arguments and parse those with argparse
Only then parse the actual command line arguments.

This way the order of precedence is correct and I think the implementation is actually much more elegant and readable!

kynan · 2022-10-08T13:31:09Z

nbstripout/_utils.py

+
+ # Traverse the file tree common to all files given as argument looking for
+ # a configuration file
+ config_path = os.path.commonpath([os.path.abspath(file) for file in args.files]) if args.files else os.getcwd()


Yes, unfortunately there is no equivalent to commonpath in pathlib. I won't insist, so you can leave as-is :)

kynan · 2022-10-08T13:40:30Z

nbstripout/_utils.py

+ if not tail:
+ break
+
+ if config:


I hadn't picked up on this in my first round review: The way you implemented this, config file settings take precedence over command line arguments, however it should be the other way around.

I realise this will be a bit harder to implement. You could turn the Namespace returned by argparse into a dict and merge that into the dict you get from reading the config files, where the argparse dict overwrites.

What makes this even more annoying is that you don't know if a value has been set by the caller or is simply the default :( I may need to look around for an idiomatic way to implement this.

Sorry about the delay. Have to prioritize something else atm. Hope to come back to this at some point but if anyone sees fit, feel free to take over.

kynan · 2022-11-14T20:16:28Z

@janosh are you still planning to pick this back up at some point?

janosh · 2022-11-14T20:23:09Z

@kynan Still pre-occupied. If @arobrien wants to take over, that would be great! 👍

…nfig-files

Not all tests working.

arobrien · 2022-12-08T14:12:45Z

@janosh and @kynan I've done some work to address the precedence and make other minor improvements. I wasn't sure how to get those edits into the PR, but I've created a PR to update this PR. Please let me know if there is a better way to address this.
janosh#1

janosh · 2022-12-08T18:02:50Z

@arobrien I think the usual approach is just to make a new PR to upstream, i.e. to https://github.com/kynan/nbstripout, not my fork. If you continued from my branch, the earlier commits will still be associated with me. I'm not worried about credit though. More important to get the feature. 😄

Either way, happy to merge your PR to my fork.

support config files contributions

kynan

Thanks, this is definitely going in the right direction :) Just a few small bits to fix.

kynan · 2022-12-09T20:39:19Z

nbstripout/_nbstripout.py

@@ -351,7 +351,7 @@ def status(git_config, install_location=INSTALL_LOCATION_LOCAL, verbose=False):
 return 1


-def main():
+def setup_commandline() -> Namespace:


This is no longer returning a Namespace but a parser, right?

Should probably be called get_parser or sth like that.

kynan · 2022-12-09T20:39:51Z

nbstripout/_utils.py

 import sys
+from collections import defaultdict
+from functools import partial
+from typing import Any, Dict, Optional



Please keep imports sorted

kynan · 2022-12-09T20:40:34Z

nbstripout/_utils.py

+ for a in parser._actions:
+ if a.dest in dict_config and isinstance(a, (_StoreTrueAction, _StoreFalseAction)):


Not a big fan of reaching into the internals of ArgumentParser but also don't have a great idea what to do instead.

Yes, I thought that this would still be more robust than keeping an explicit list of Boolean arguments, although that wouldn't be impossible. I looked at a range of options, including shifting to an alternative arguments library, like Click, but that all got too complicated.
I think that this is a reasonable compromise if we keep on top of adding new Python versions to the test suite whenever they become available.

kynan · 2022-12-09T20:41:25Z

nbstripout/_utils.py

+ for a in parser._actions:
+ if a.dest in raw_config and isinstance(a, (_StoreTrueAction, _StoreFalseAction)):


kynan · 2022-12-09T20:43:20Z

nbstripout/_utils.py

+ # when it finds the file, or a .git directory, or a .hg directory, or the root of the file system, whichever
+ # comes first.
+ # if no files are given, start from cwd
+ config_path = os.path.commonpath([os.path.abspath(file) for file in args.files]) if args.files else os.path.abspath(os.getcwd())


This line gets a bit long with a ternary

nbstripout/_utils.py

kynan · 2022-12-09T21:00:17Z

nbstripout/_utils.py

+ if name not in valid_args:
+ raise ValueError(f'{name} in the config file is not a valid option')
+
+ # separate into default-overrides and special treatment


Please add a brief comment why extra_keys needs special treatment.

I'm not 100% sure that it does. I left this with the same functionality that @janosh did.
This special treatment lets you specify a base set of extra keys in the config files, and then add to that in an ad-hoc manner, but does not let you remove extra keys....
Alternative treatment would be that whenever you specify extra keys you would have to list the complete set.
The more I think about it, the more I think that perhaps special treatment is NOT justified.

Yes, I think that's a fair point and treating it specially may lead to surprising results.

@janosh can you explain your rationale? Do you agree that special treatment of this flag may be surprising?

I don't remember for sure at this point, but I think it might be left over from the original autoflake8 config file PR. I agree minimal user surprise is usually the way to go.

tests/test_read_config_files.py

Co-authored-by: Florian Rathgeber <[email protected]>

* 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 kynan#187

kynan · 2023-12-07T21:09:16Z

nbstripout/_nbstripout.py

@@ -120,7 +120,7 @@
 import sys
 import warnings

-from nbstripout._utils import strip_output, strip_zeppelin_output
+from nbstripout._utils import strip_output, strip_zeppelin_output, merge_configuration_file


Suggested change

from nbstripout._utils import strip_output, strip_zeppelin_output, merge_configuration_file

from nbstripout._utils import strip_output, strip_zeppelin_output, merge_configuration_files

kynan · 2023-12-07T21:09:27Z

nbstripout/_nbstripout.py

+
+def main():
+ parser = setup_commandline()
+ args = merge_configuration_file(parser)


Suggested change

args = merge_configuration_file(parser)

args = merge_configuration_files(parser)

kynan · 2023-12-07T21:11:06Z

@janosh Are you still planning to address the outstanding review comments? :)

janosh · 2023-12-07T21:16:47Z

Sorry, don't have time. Feel free to close.

kynan · 2023-12-07T21:18:15Z

Thanks for the update! I'll leave this pending for now. Maybe I'll find time to pick this up myself over the holidays.

l-johnston · 2024-01-03T14:38:45Z

Will this feature be available in pyproject.toml?

In my use case, I need to filter notebooks that have a specific cell tag while leaving all others unfiltered. Will this feature support this use case?

kynan · 2024-01-26T22:55:23Z

Will this feature be available in pyproject.toml?

I think this PR was supposed to add pyproject.toml support for configuring nbstripout, yes.

I need to filter notebooks that have a specific cell tag while leaving all others unfiltered. Will this feature support this use case?

I don't think your feature request is related to this PR. Please file a new issue.

@l-johnston would you be interested in working on this PR?

kynan requested changes Oct 2, 2022

View reviewed changes

This was referenced Oct 2, 2022

Fix typos #170

Merged

Bump CI actions/checkout to v3 and actions/setup-python to v4 #171

Merged

Fix shellcheck SC2006 in pre-commit script #172

Merged

Replace sys.exit() with raise SystemExit #173

Merged

janosh added 3 commits October 2, 2022 07:17

add process_pyproject_toml(), process_setup_cfg(), merge_configuratio…

b2d4bbd

…n_file() in nbstripout/_utils.py

call merge_configuration_file() in nbstripout/_nbstripout.py after pa…

758cebc

…rser.parse_args()

add tests/test_read_config_files.py

e5cd304

janosh force-pushed the support-config-files branch from 09716a9 to e5cd304 Compare October 2, 2022 14:17

make type hints py36 compatible

5d45bd0

kynan requested changes Oct 8, 2022

View reviewed changes

arobrien mentioned this pull request Nov 14, 2022

Can I use nbstripout in Github action as a test ? #158

Open

kynan assigned janosh Nov 14, 2022

kynan added type:enhancement state:waiting Waiting for response for reporter state:needs follow up This issue has interesting suggestions / ideas worth following up on labels Nov 14, 2022

kynan added this to the Backlog milestone Nov 14, 2022

Andrew O'Brien added 6 commits December 7, 2022 19:17

Merge remote-tracking branch 'official/master' into janosh/support-co…

c4e073c

…nfig-files

re-design config reading to give command line precedence.

4c500c9

Not all tests working.

fix handling of non_bool values in pyproject.toml

92440f1

fix no file arguments tests

dce4163

test argument and file precedence

7d02b41

flake8

0a75bc1

Merge pull request #1 from arobrien/janosh/support-config-files

4a61903

support config files contributions

kynan requested changes Dec 10, 2022

View reviewed changes

kynan modified the milestones: Backlog, 0.7.0 Dec 10, 2022

janosh and others added 6 commits December 26, 2022 15:07

Update nbstripout/_utils.py

c7abff0

Co-authored-by: Florian Rathgeber <[email protected]>

Update tests/test_read_config_files.py

e8ced46

Co-authored-by: Florian Rathgeber <[email protected]>

Add python3.11 classifier to setup.py, remove python3.6 (kynan#186)

749f431

Improve documentation for notebook and cell metadata stripping

7b065ef

* Clarify that extra keys may _only_ specify notebook and cell metadata to be stripped. * Update metadata stripped by default. Addresses kynan#187

Merge branch 'master' into support-config-files

56bcd02

kynan requested changes Dec 7, 2023

View reviewed changes

kynan modified the milestones: 0.7.0, Backlog Jan 27, 2024

kynan force-pushed the master branch from 8545b87 to 4368a01 Compare February 3, 2024 09:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support config files #169

Support config files #169

janosh commented Oct 1, 2022

kynan left a comment

kynan Oct 2, 2022

janosh Oct 2, 2022

kynan Oct 8, 2022

stuaxocabinetoffice Apr 5, 2024

kynan commented Oct 2, 2022

kynan left a comment

kynan Oct 8, 2022

kynan Oct 8, 2022

janosh Oct 27, 2022

kynan commented Nov 14, 2022

janosh commented Nov 14, 2022

arobrien commented Dec 8, 2022

janosh commented Dec 8, 2022

kynan left a comment

kynan Dec 9, 2022

kynan Dec 9, 2022

kynan Dec 9, 2022

kynan Dec 9, 2022

arobrien Dec 11, 2022

kynan Dec 11, 2022

kynan Dec 9, 2022

kynan Dec 9, 2022

kynan Dec 9, 2022

arobrien Dec 11, 2022

kynan Dec 11, 2022

janosh Dec 11, 2022

kynan Dec 7, 2023

kynan Dec 7, 2023

kynan commented Dec 7, 2023

janosh commented Dec 7, 2023

kynan commented Dec 7, 2023

l-johnston commented Jan 3, 2024

kynan commented Jan 26, 2024

		for a in parser._actions:
		if a.dest in dict_config and isinstance(a, (_StoreTrueAction, _StoreFalseAction)):

		for a in parser._actions:
		if a.dest in raw_config and isinstance(a, (_StoreTrueAction, _StoreFalseAction)):

	from nbstripout._utils import strip_output, strip_zeppelin_output, merge_configuration_file
	from nbstripout._utils import strip_output, strip_zeppelin_output, merge_configuration_files

	args = merge_configuration_file(parser)
	args = merge_configuration_files(parser)

Support config files #169

Are you sure you want to change the base?

Support config files #169

Conversation

janosh commented Oct 1, 2022

kynan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kynan commented Oct 2, 2022

kynan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kynan commented Nov 14, 2022

janosh commented Nov 14, 2022

arobrien commented Dec 8, 2022

janosh commented Dec 8, 2022

kynan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kynan commented Dec 7, 2023

janosh commented Dec 7, 2023

kynan commented Dec 7, 2023

l-johnston commented Jan 3, 2024

kynan commented Jan 26, 2024