Skip to content

Add ability to override raw input #654

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

Merged
merged 10 commits into from
Aug 8, 2025
Merged

Add ability to override raw input #654

merged 10 commits into from
Aug 8, 2025

Conversation

DannyBen
Copy link
Member

@DannyBen DannyBen commented Aug 6, 2025

cc #643

This PR allowed overriding the raw input args before they are fed into run.
It mainly changes the script startup to this:

  command_line_args=("$@")
  initialize
  run "${command_line_args[@]}"

  • Proof of concept
  • Specs pass
  • Final implementation
  • Example or integration fixture
  • Documentation - source / preview

@DannyBen
Copy link
Member Author

DannyBen commented Aug 6, 2025

@mcblum @pcrockett
This is the best way I found to allow advanced developers to "hack" the raw input line, and replace it if they want.
It allows replacing it in the initialize, by declaring an array named input_override.

@meleu
if you wish to chime in, you are welcome as usual

@DannyBen DannyBen added this to the 1.3 milestone Aug 6, 2025
@mcblum
Copy link
Collaborator

mcblum commented Aug 6, 2025

This looks great to me, thank you for getting after it so quickly!

@DannyBen
Copy link
Member Author

DannyBen commented Aug 6, 2025

Will this solve your use case?

@pcrockett
Copy link
Collaborator

hm, i dunno. i think the VAST majority of scripts out there have no need to manipulate raw args. so adding a logic branch just for this one edge case seems a bit wonky to me.

i tend to lean more toward something like this:

raw_args="$@"
initialize # has ability to manipulate raw_args if desired
run "${raw_args[@]}"

this wouldn't clutter up my scripts so much, but would give "edge-case" folks the flexibility they want. wdyt?

@DannyBen
Copy link
Member Author

DannyBen commented Aug 7, 2025

so adding a logic branch just for this one edge case seems a bit wonky to me.

Yes, thank you. Totally agree.
So you do not object to the idea, just the implementation - I will update it to match yours.

@DannyBen
Copy link
Member Author

DannyBen commented Aug 7, 2025

Done. I called the variable command_line. I wanted to call it input but it would have conflicted with the declare -g -a input=() in run().

I wonder if this needs documentation, or having it as an example is sufficient - given the fact that this is a not-recommended-edge-case. Added just minimal documentation in the Function Hooks section, as I do not want to encourage this usage pattern.

@DannyBen
Copy link
Member Author

DannyBen commented Aug 7, 2025

Ready to merge?

pcrockett
pcrockett previously approved these changes Aug 7, 2025
Copy link
Collaborator

@pcrockett pcrockett left a comment

Choose a reason for hiding this comment

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

Looks good to me. Small nit, not blocking a merge:

I'm not an enormous fan of the command_line name, as that implies to me that it includes the full command (including the script name / path). Something with "args" in the name (i.e. command_line_args) makes more sense to me. It's a matter of preference / taste though.

And other by-the-way comments:

  • I'm not sure how you like to handle PRs, but you might consider updating the original description with the current state of the PR, so future people don't need to read all the way through to know what actually got merged.
  • I actually unsubscribed from this PR because I was getting too many notifications (every time you pushed a commit -- not your fault, just GitHub's). I luckily checked it again today and saw you asked me for another look. In the future, (re-)requesting a review from me would be more likely to get my attention.

@DannyBen DannyBen merged commit dfa1a2b into master Aug 8, 2025
7 checks passed
@DannyBen DannyBen deleted the add/input-override branch August 8, 2025 04:35
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