Skip to content

reactor: GitHub Action for improved notebook file handling - #212

Merged
kynan merged 8 commits into
kynan:mainfrom
kunalagra:refactor/gh-actions-refactor
Apr 11, 2026
Merged

kynan merged 8 commits into
kynan:mainfrom
kunalagra:refactor/gh-actions-refactor

Conversation

@kunalagra

Copy link
Copy Markdown
Contributor

This PR refactors the GitHub composite action in action.yml to make notebook detection and verification safer and more robust across common runner environments.

Fixes #211

What Changed

  1. Removed the separate "find notebooks" step and moved discovery into the verification step.
  2. Replaced shell-based path discovery with Python glob discovery:
  • Handles recursive patterns like **/*.ipynb reliably.
  • Prevents shell pre-expansion issues from user-provided path patterns.
  • Produces null-delimited paths for safe downstream processing.
  1. Switched command construction to bash array args instead of string/eval composition:
  • Avoids quoting/eval edge cases.
  • Keeps input flag handling explicit and safer.
  1. Added input mapping through environment variables (INPUT_*) for clearer script logic.
  2. Added explicit no-match behavior:
  • If no notebooks match, action exits successfully with an informative message.
  1. Kept verification execution scalable by using xargs -0 with null-delimited file paths.
  2. Preserved existing action inputs and behavior contract:
  • paths
  • extra-keys
  • keep-output
  • keep-count
  • strip-init-cells

@kunalagra

kunalagra commented Mar 11, 2026 •

Copy link
Copy Markdown
Contributor Author

Testing Performed at:- https://github.com/kunalagra/nbstripout-action-test

Comment thread action.yml
Comment thread action.yml
@kynan

kynan commented Mar 14, 2026

Copy link
Copy Markdown
Owner

Are you sure your revised action actually works with spaces in filenames? In this run I see

No notebook files found matching: My Analysis (draft).ipynb

@kunalagra

kunalagra commented Mar 15, 2026 •

Copy link
Copy Markdown
Contributor Author

Are you sure your revised action actually works with spaces in filenames? In this run I see

No notebook files found matching: My Analysis (draft).ipynb

That is indeed correct. It seems to have been missed by me. The action works when the file is part of glob matching and not passes as filename to the input of the action as the input does split by spaces. Run - https://github.com/kunalagra/nbstripout-action-test/actions/runs/22976128179/job/66705053665

I think the ideal solution would be to pass them as multi line string instead of the current space based filename to the action? Would also update README to reflect the change.

@kynan

kynan commented Mar 15, 2026

Copy link
Copy Markdown
Owner

I think the ideal solution would be to pass them as multi line string instead of the current space based filename to the action? Would also update README to reflect the change.

OK, I have no particular opinion here. Any behaviour changes, please update the README :)

@kunalagra

kunalagra commented Mar 15, 2026 •

Copy link
Copy Markdown
Contributor Author

@kunalagra
kunalagra requested a review from kynan March 15, 2026 18:57
@lagamura

Copy link
Copy Markdown
Contributor

@kunalagra thanks you very much for the suggested changes, are you aware if we can add testing cases for the repo's action, alike at your branch? Maybe it's an overkill but I've never bothered adding auto-testing for an action before and was curious.

Also, if you are aware of any channels we could promote the nbstripout re-usable action, let me know :)
As I think there is not anything open-source, based in a mature hook like nbstripout, out there.

@kunalagra

kunalagra commented Mar 17, 2026 •

Copy link
Copy Markdown
Contributor Author

We could definitely add tests for the action. Since the repo already has e2e notebooks I think we can have GitHub Action that would run on those and define cases? Let me check on this.

Also, if you are aware of any channels we could promote the nbstripout re-usable action, let me know :)

I am not aware of such channels. Maybe Data Science/ML subreddits or groups? Need to look into.

@kunalagra

Copy link
Copy Markdown
Contributor Author

@kynan have added the test action that will use the repo's path to action file. Also have copied files and renamed them to confirm the workflow.

@kunalagra

Copy link
Copy Markdown
Contributor Author

Ping

@kynan kynan left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks, this is looking good now! Sorry for the delay, was travelling for a while.

Comment thread action.yml
@kynan
kynan merged commit 3740130 into kynan:main Apr 11, 2026
19 checks passed
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.

GitHub Action fails on filenames with spaces or parentheses

3 participants