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

DevEx: Implement the argument parser in our typescript scripts #5654

Merged
merged 57 commits into from
Jan 10, 2025

Conversation

jimlerza
Copy link
Collaborator

@jimlerza jimlerza commented Dec 17, 2024

See #5606 for the fancy marketing blahblah

Successful migration deployment

jimlerza and others added 30 commits December 5, 2024 15:42
Copy link
Contributor

@zachrog zachrog left a comment

Choose a reason for hiding this comment

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

Overall this looks good and helps us standardize how we run our scripts. Thank you for cleaning all of this up. Big fan.

Personally, I think that handling the argument parsing should be done by a package rather than rolling our own, or custom functionality like "requireActiveAwsSession" be built on top of a package. Maintaining our own arg parser does not seem valuable to me, however I recognize the court is the one who uses scripts far more often than Flexion so trust your judgement and needs more than my own. Definitely not a show stopper but wanted to be direct.

@jimlerza
Copy link
Collaborator Author

jimlerza commented Jan 8, 2025

Overall this looks good and helps us standardize how we run our scripts. Thank you for cleaning all of this up. Big fan.

Personally, I think that handling the argument parsing should be done by a package rather than rolling our own, or custom functionality like "requireActiveAwsSession" be built on top of a package. Maintaining our own arg parser does not seem valuable to me, however I recognize the court is the one who uses scripts far more often than Flexion so trust your judgement and needs more than my own. Definitely not a show stopper but wanted to be direct.

Argument parsing is done by node:utils, which is not a package, but built into node itself. We started implementing that in a few scripts, and it worked well, but it required a lot of code to set up the object and then get the values out, so I abstracted most of that into a wrapper.

This is a follow-up PR which implements that wrapper in the scripts. The original PR was just the wrapper and its tests.

@jimlerza jimlerza marked this pull request as ready for review January 8, 2025 19:15
@jimlerza jimlerza merged commit fa6858f into staging Jan 10, 2025
57 checks passed
@jimlerza jimlerza deleted the devex-argument-parser-implemented branch January 10, 2025 22:15
@jimlerza jimlerza mentioned this pull request Jan 25, 2025
4 tasks
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.

3 participants