Move argv override logging to middleware #42
Conversation
Generate changelog in
|
| `Overriding "directory" from config file with ${argv.directory}`, | ||
| if ( | ||
| autoVersion != null && argv.autoVersion !== autoVersion.type | ||
| && argv.autoVersion !== "git-describe" |
There was a problem hiding this comment.
Minor note I feel this block and the one above are going to be difficult to maintain if we add another auto version type. The checks also seem slightly loose for example the above one will throw with --autoVersion git-describe --tagPrefix abc if there is no config file but that would be a valid state
There was a problem hiding this comment.
Yeah I agree, but it is required I think to not get the user in a state that doesn't make sense.
As for the part that didn't work, that was just because I forgot to check if autoVersion was defined.
Open to ideas here honestly to make this easier to handle.
There was a problem hiding this comment.
I would recommend breaking up the check a bit for example:
const autoVersionType = argv.autoVersion ?? autoVersion;
if (autoVersionType !== "git-describe") { throw }
To make it extensible in the future we may be able to do something with the AutoVersionType type and visit against each of the cases in the union and if we don't match any then we would throw
| ); | ||
| } | ||
|
|
||
| if (gitTagPrefix != null && argv.gitTagPrefix !== gitTagPrefix) { |
There was a problem hiding this comment.
Let's also make these checks tighter currently this is going to say misleading things like: Overriding "gitTagPrefix" from config file with undefined
There was a problem hiding this comment.
Which command did you run to get to that state?
There was a problem hiding this comment.
This is because I have "tagPrefix": "" in my config file which I think would be good to illustrate that the key exists if someone discovers it and wants to edit it. We probably have some loose truthy checks on strings which is why some people use === everywhere
| `Overriding "directory" from config file with ${argv.directory}`, | ||
| if ( | ||
| autoVersion != null && argv.autoVersion !== autoVersion.type | ||
| && argv.autoVersion !== "git-describe" |
There was a problem hiding this comment.
I would recommend breaking up the check a bit for example:
const autoVersionType = argv.autoVersion ?? autoVersion;
if (autoVersionType !== "git-describe") { throw }
To make it extensible in the future we may be able to do something with the AutoVersionType type and visit against each of the cases in the union and if we don't match any then we would throw
| @@ -92,7 +92,7 @@ const command: CommandModule< | |||
| } | |||
|
|
|||
| if ( | |||
| (autoVersion?.type !== "git-describe" | |||
| ((autoVersion != null && autoVersion.type !== "git-describe") | |||
There was a problem hiding this comment.
maybe:
const autoVersionType = argv.autoVersion ?? autoVersion;
if (argv.gitTagPrefix != null && autoVersionType !== "git-describe") { ... }
* origin/main: Autorelease Increase CLI max width to 150 columns (#47) Debug log site zip stats (#49) Clean up yargs and consola imports (#51) Autorelease Add generated changelog entries Breaking: force dollar sign in osdk verbs for where Display errors more consistently in @osdk/cli (#41) Fix tagPrefix Add generated changelog entries Create foundry.config.json in create-app [3/] Improvement: Use loadedToken for fetching data (#17) Move argv override logging to middleware (#42) Make sure to run all tests (#39) Update create-app to use FOUNDRY_TOKEN in templates (#40) Add generated changelog entries Fix intellisense for aggregations and fill in more number types [2/] Improvement: Introduce new command structure (#16) Autorelease Refactor create-app prompts with tests (#32) Refactor create-app dotfile generation (#31) add changelog Add generated changelog entries Add generated changelog entries put mkdir in right place remove generated files add ignore for legacy client remove deleted file added add back mkdir move mkdir fix fixes keep trying remove module shared testing infra for legacy
Addresses #16 (comment)