Update seedCacheFile.ts #1162
Update seedCacheFile.ts #1162
Conversation
There was a problem hiding this comment.
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.
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.
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.
@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.
Is there a reason this is replacing the default prompt behavior for daysPerRequest?
There was a problem hiding this comment.
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.
@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.