-
Notifications
You must be signed in to change notification settings - Fork 281
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
Update seedCacheFile.ts #1162
base: trunk
Are you sure you want to change the base?
Update seedCacheFile.ts #1162
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this PR @Robvred! I believe it will definitely be useful to have the option to bypass the interactive prompts in place for flagged options, and we appreciate you being willing to contribute this.
I listed a few questions/concerns in my review that would need to be addressed prior to merging. I also want to point out that while I believe it is good to have flagged options/parameters, we probably don't want to remove the interactive prompts. I'd recommend referring to the main cli.ts
file to see how we go about providing interactive options alongside flagged ones.
Lastly, there are a few linting errors and merge conflicts checks that will need to be resolved before I'm able to merge.
Feel free to let me know if you have any questions/thoughts on any of the above!
import { App, createValidFootprintRequest } from '@cloud-carbon-footprint/app' | ||
import yargs from 'yargs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is currently only available as a subdependency. We would need to add this as a dependency via yarn add yargs
to avoid any issues for people installing the cli package independently.
However, we currently already use the Commander package to allow for flag options in the main cli program (packages/cli/src/cli.ts
). It may be better to keep consistency by using this same package here rather than introducing a new one -- unless an argument can be made to use this one in its place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @4upz you totaly right, these mistakes of considerations of the whole sub-dependancies with other sub programs of ccf, come from a lack of understanding of how it's has been implemented.
Initialy when I've looked at how it's has been seeting up, I've understood than seed-cache was like autonomous in the way it works.
So if CLI program currently uses commander
for flag options, so it might be better to use commander
for consistency as I don't see specific reason to use yargs
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Robvred No worries on the lack of understanding! The CLI has a lot going on. The Seed Cache file serves as a utility script or alternative way of fetching estimates to the main CLI command.
Let's definitely keep them consistent. If you don't mind updating the use of args to Commander instead, that'd be awesome.
await new Promise((resolve) => { | ||
process.stdin.once('data', (data) => { | ||
resolve(data.toString().trim()) | ||
}) | ||
console.log('How many days would you like to fetch per request? [1]') | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this is replacing the default prompt behavior for daysPerRequest
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The replacement of inputPrompt method with a promise make the code more flexible and less dependent on a specific method for reading user input.
However, I have to admit after a few nights of sleep, now it's than the new code does not validate the user input to ensure that it is a valid integer....
So both options are viable and it depends on how you would like to handle user input validation.
Which one do you "prefer" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Robvred That makes sense. I'd lean on the side of keeping the function used for receiving user input consistent, and using the one with input validation already implemented. This way validation is consistent, and also if we decide we'd like to change/update the behavior then we can do so from the single inputPrompt method rather than updating more than one place.
Hi @Robvred, are you still interested to continue working on this PR? |
Description of Change
With this code, you can run the script using named options with values, like this:
yarn seed-cache-file --start 2022-01-01 --end 2022-01-31 --groupBy month --fetchMethod split --cloudProviderToSeed aws
Checklist
Notes
Additional information relevant to the changes
© 2021 Thoughtworks, Inc.