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

feat: [#631] implemented refOptions CLI argument #632

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ndopj
Copy link

@ndopj ndopj commented Sep 11, 2024

resolves: #631

Current changes accept $refOptions argument as JSON string from command line.

Those options are then being merged with default value for $refOptions. Even if current default value {} would be changed in the future, fields from command line argument --refOptions won't influence "not overridden" $refOption fields.

Command line argument --refOptions cannot use 1:1 naming with Options interface field $refOptions because it contains $ character which might cause issues (on some Shells), if the command line arguments are not being enclosed as string ("").

Still need to figure out if its possible to create any meaningful tests for this !!
Any help or feedback appreciated.

@ndopj ndopj changed the title feat: [631] implemented refOptions CLI argument feat: [#631] implemented refOptions CLI argument Sep 11, 2024
@ndopj ndopj force-pushed the ndopj/631-cli-add-refoptions branch from de85b7f to 116708b Compare September 11, 2024 00:25
@ndopj
Copy link
Author

ndopj commented Sep 11, 2024

@bcherny Could I kindly ask for a review ? Don't know anybody amongst contributors so tagging you as an active one. I hope its not rude to you, in that case I am sorry 😶

Also if test should be included for this CLI option - I am not sure whether this would fit testCLI.ts structure. If I understand it correctly, tests are implemented via snapshots meaning it's asserted that output of each tested operation is same ? However, in such case, it would not be possible to include scenario described in the linked issue. I am correct ?

@ndopj ndopj force-pushed the ndopj/631-cli-add-refoptions branch from 984bb9c to 900da3f Compare September 11, 2024 07:45
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.

Allow configuration of $refOptions in CLI application
1 participant