Skip to content

reactor: GitHub Action for improved notebook file handling#212

Open
kunalagra wants to merge 8 commits intokynan:mainfrom
kunalagra:refactor/gh-actions-refactor
Open

reactor: GitHub Action for improved notebook file handling#212
kunalagra wants to merge 8 commits intokynan:mainfrom
kunalagra:refactor/gh-actions-refactor

Conversation

@kunalagra
Copy link
Copy Markdown

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
Copy link
Copy Markdown
Author

kunalagra commented Mar 11, 2026

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

@kynan
Copy link
Copy Markdown
Owner

kynan commented Mar 14, 2026

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
Copy link
Copy Markdown
Author

kunalagra commented Mar 15, 2026

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
Copy link
Copy Markdown
Owner

kynan commented Mar 15, 2026

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
Copy link
Copy Markdown
Author

kunalagra commented Mar 15, 2026

@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
Copy link
Copy Markdown
Author

kunalagra commented Mar 17, 2026

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
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.

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.

GitHub Action fails on filenames with spaces or parentheses

3 participants