Ensure all options can be specified via CLI #31

TimothyJones · 2022-06-25T02:37:01Z

Some options can only be specified in the config files. Both #28 and conventional-changelog#912 would be solved if the type of the updater could be specifed via CLI.

Since the options for commit-and-tag-version are not complicated, it would totally be possible to extend the CLI options to make sure that everything that can be specified by a config file can also be specified via CLI.

Ideally this would be done without embedding json in the CLI options.

The text was updated successfully, but these errors were encountered:

helmturner · 2023-03-22T06:19:39Z

(Follow-up on #29 (comment) and #29 (comment)):

Below is a breakdown of current config options. Please let me know if you see any errors or ommissions.

Under the "CLI" column are the valid CLI flags, where:
- < > denotes options which require an arg
- [ ] denotes that the option can can be passed without an arg
- <...> denotes an array of args
- { } denotes the use of dot-notation to specify nested properties
Under the JSON column is a type annotation that represents the shape expected by the program (there's a type annotation legend below).

Option	CLI	JSON	Notes
packageFiles	`--packageFiles <string...>`	packageFiles: `ConfigFiles`	Only the path name can be specified on CLI, see #28
bumpFiles	`--bumpFiles <string...>`	bumpFiles: `ConfigFiles`	Only the path name can be specified on CLI, see #28
releaseAs	`-r, --release-as <Release>`	releaseAs: `Release`	Note the inconsistency between camelCase and kebab-case on CLI
prerelease	`--prerelease [string]`	prerelease: `string \| boolean`	While the arg must be a string if passed, it defaults to `true`
infile	`-i, --infile <string>`	infile: `string`	__
message	`-m, --message <string>`	message: `string`	Per `--help`, deprecated (use `--releaseCommitMessageFormat`)
firstRelease	`-f, --first-release`	firstRelease: `boolean`	__
sign	`-s, sign`	sign: `boolean`	__
noVerify	`-n, no-verify`	noVerify: `boolean`	__
commitAll	`-a, commit-all`	commitAll: `boolean`	__
silent	`--silent`	silent: `boolean`	__
tagPrefix	`--tag-prefix [string]`	tagPrefix: `string`	"v" if no arg passed - for no tag, use empty string ("")
releaseCount	`--release-count <number>`	releaseCount: `number`	__
tagForce	`--tag-force`	tagForce: `boolean`	__
scripts	`--scripts.{Hook} <string>`	scripts: Record<Hook, string>	__
skip	`--skip.{Task}`	skip: Record<Task, string>	__
dryRun	`--dry-run`	dryRun: `boolean`	__
gitTagFallback	`--git-tag-fallback`	gitTagFallback: `boolean`	__
path	`--path <string>`	path: `string`	__
changelogHeader	`--changelogHeader <string>`	changelogHeader: `string`	Per '--help', deprecated (use `--header`)
preset	`--preset <string>`	preset: `string`	__
lernaPackage	`--lerna-package <string>`	lernaPackage: `string`	__
npmPublishHint	`--npmPublishHint <string>`	npmPublishHint: `string`	__
header	`--header <string>`	header: `string`	__
types	N\A	types: `TypePrefixes`	`--help` says `--types` is valid on CLI; It's silently ignored.
preMajor	`--preMajor`	preMajor: `boolean`	__
commitUrlFormat	`--commitUrlFormat <string>`	commitUrlFormat: `string`	__
compareUrlFormat	`--compareUrlFormat <string>`	compareUrlFormat: `string`	__
issueUrlFormat	`--issueUrlFormat <string>`	issueUrlFormat: `string`	__
userUrlFormat	`--userUrlFormat <string>`	userUrlFormat: `string`	__
[message format]	`--releaseCommitMessageFormat <string>`	releaseCommitMessageFormat: `string`	__
issuePrefixes	`--issuePrefixes <string...>`	issuePrefixes: `string[]`	__

LEGEND:

type Release = "minor" | "major" | "patch";
type Task = "changelog" | "commit" | "tag";
type Hook =
  | "prerelease"
  | "prebump"
  | "postbump"
  | "prechangelog"
  | "postchangelog"
  | "precommit"
  | "postcommit"
  | "pretag"
  | "posttag";

type TypePrefixes = Array<
  (
    | { section: string; hidden?: boolean | undefined }
    | { hidden: true; section?: string | undefined }
  ) & { type: string }
>;

type ConfigFiles = Array<
  | string
  | { filename: string; type: "json" | "gradle" | "plain-text" }
  | { filename: string; updater: string }
>;

Some thoughts

Ultimately, the difference between the CLI and JSON options are actually rather minimal. Only 1 option is wholly unavailable via CLI (unless I'm just not getting the syntax right), and 2 more are limited in functionality. How much work are those 3 options worth?

Currently, some config options are dynamically generated by reading in the JSON schema for the conventional-changelog config spec. This conveniently keeps the JSON configs up-to-date with the standard, but it also means necessarily that we must support JSON in the CLI to get full functionality for the 3 options in question.

The alternative would be to manually maintain the CLI implementation separate from the parsed JSON config and keep it up-to-date as the spec changes. The up-front work is higher, but I don't expect the spec changes frequently enough to present a huge maintenance burden. The upside is that we can get more creative:

# lone strings are assumed to be filenames, and filenames delimit array items
$ commit-and-tag-version --packageFiles package.json manifest-beta.json type=json MY_VERSIONS.json updater=my-updater.js name=gradle.properties type=gradle

# or
$ commit-and-tag-version --packagFiles 1=package.json 2=manifest-beta.json 2.type=json 3=MY_VERSIONS.json 3.updater=my-updater.js 4.name=gradle.properties 4.type=gradle

# or, since `type and `updater` are mutually exclusive, we can use a single key and infer it as an updater if it isn't `json`, `gradle`, or `plain-text`
$ commit-and-tag-version --packageFile package.json --packageFile manifest-beta.json json --packageFile MY_VERSIONS.json my-updater.js --packageFile gradle.properties gradle

PS: Last minute idea... what if everything is the same, but for those 3 args we add new CLI flags --package-file (for packageFiles), --bump-file (for bumpFiles), and --type-prefix (for types). Nothing breaks, we get feature parity, and everyone wins... or am I being naively optimistic again?

TimothyJones · 2023-03-22T12:21:23Z

This is awesome!! Thank you so much!

I'll have a proper read and detailed review tomorrow (in around 12 hours).

My gut feel is that I like both the manual separate CLI suggestion (I think that separating the CLI parsing logic out from the options format will be a win), and the last minute idea (it seems simple).

What's really missing at the moment is the ability to easily combine the two sources of options - I think the combineConfig(cliConfig, packageJsonConfig) thing I suggested on the other issue would be a key win - then we could trivially expand and contract the different sources of options, and it wouldn't matter if people supplied some options via config, and others via CLI.

I think the key wins here will be:

Options not overwriting the ones from different config sources
Trivial to add options and have them available in all config places
Play nicer with typescript for the issue you originally wanted to work on, of course 😎

Aside: I started looking at Commander for another project - looks awesome, thanks for the recommendation!

TimothyJones · 2023-03-22T12:28:12Z

Note the inconsistency between camelCase and kebab-case on CLI

Great catch! If possible, let's fix this.

Maybe we can support both but leave one undocumented, so that it's not a breaking change?

Also, I'm very aware this is becoming a sizeable piece of yak shaving 😂 . Please let me know if you'd like to reduce implementation workload by working on a branch together. Of course, if you'd prefer to just raise a PR when you're ready, that's fine too - I don't mind either way.

TimothyJones · 2023-03-25T00:51:56Z

In #61, it was reported that (currently) supplying invalid options doesn't cause the command to fail. Looks like moving to commander will fix that for free, though 🎉

helmturner · 2023-03-25T02:18:17Z

This is awesome!! Thank you so much!

I'll have a proper read and detailed review tomorrow (in around 12 hours).

My gut feel is that I like both the manual separate CLI suggestion (I think that separating the CLI parsing logic out from the options format will be a win), and the last minute idea (it seems simple).

What's really missing at the moment is the ability to easily combine the two sources of options - I think the combineConfig(cliConfig, packageJsonConfig) thing I suggested on the other issue would be a key win - then we could trivially expand and contract the different sources of options, and it wouldn't matter if people supplied some options via config, and others via CLI.

I think the key wins here will be:

Options not overwriting the ones from different config sources

Trivial to add options and have them available in all config places

Play nicer with typescript for the issue you originally wanted to work on, of course 😎

Aside: I started looking at Commander for another project - looks awesome, thanks for the recommendation!

Any thoughts on the order of precedence? Here's where I'm at:

It seems intuitive to me that the CLI has the highest precedence, i.e. it overrides all static configurations.
I can imagine a use-case for package.json being the lowest precedence, e.g. you might have a template repo or init script with a default package.json config that you would like to override as needed via config files.
I think (await import('package.json'))["commit-and-tag-version"] should take precedence over (await import('package.json'))["standard-version"].

I don't know if there's a certain ordering that is more or less intuitive to folks, especially when it comes to the order of precedence between .versionrc, .versionrc.js, and .versionrc.json.

There's also the option of making certain config sources mutually exclusive - e.g., throw if there is more than one of .versionrc, .versionrc.js, .versionrc.json, or more than one of "standard-version", "commit-and-tag-version" in package.json.

SharpSeeEr · 2023-03-28T18:09:33Z

For the config files I would say precedence is irrelevant, but the order they are looked for is. I would look for .versionrc, then .versionrc.js, and finally .versionrc.json. First one that is found is the one that is used.

I think the same logic could be applied to package.json. First look for commit-and-tag-version, then only look for standard-version if the first isn't found.

That's how I have seen other utilities work anyway.

helmturner · 2023-03-29T16:24:21Z

@SharpSeeEr Is this a convention? If so, I'm all for sticking to the convention.

I feel like I agree with you on the former point, but not on the latter. That's more of a gut feel though, so I'd love to see more feedback.

Edit: It also occurs to me that, with #29 on the roadmap, we should enable (and recommend) versionrc.ts

SharpSeeEr · 2023-03-29T18:02:28Z

I wouldn't be able to say if it is a standard convention or not. I have seen it done like that on at least one project. 😸

I'll concede the second point no problem.

As for versionrc.ts, when they run commit-and-tag-version in their project, it's already been compiled to typescript. If they provide a .ts config file, it would have to be transpiled before being imported/read.

helmturner · 2023-03-30T01:29:09Z

As for versionrc.ts, when they run commit-and-tag-version in their project, it's already been compiled to typescript. If they provide a .ts config file, it would have to be transpiled before being imported/read.

When they run it, yes... but all TS is JS at runtime. The benefit is the in-editor documentation via IntelliSense. Targeting versionrc.ts for transpilation is simple enough via tsconfig.json. It's an additive change - just a nicety for the folks like me that would appreciate it.

TimothyJones · 2023-03-30T01:44:07Z

I don't think there's much convention. For example, eslint's precedence order is:

.eslintrc.js
.eslintrc.cjs
.eslintrc.yaml
.eslintrc.yml
.eslintrc.json
package.json

Prettier's order is the reverse:

A "prettier" key in your package.json file.
A .prettierrc file written in JSON or YAML.
A .prettierrc.json, .prettierrc.yml, .prettierrc.yaml, or .prettierrc.json5 file.
A .prettierrc.js, .prettierrc.cjs, prettier.config.js, or prettier.config.cjs file that exports an object using module.exports.
A .prettierrc.toml file.

Both tools appear to pick one and then stop, which I didn't realise until finding those links just now.

I don't think the order matters too much, as long as it's documented and not especially surprising (and not overly complicated from an implementation perspective).

helmturner · 2023-03-30T02:31:09Z

Right, then - we pick something and document it. Unless someone has a good reason for bikeshedding it, guess we can go with @SharpSeeEr's suggestion. That gives us:

CLI args
Config file
- [.versionrc > .versionrc.json > .versionrc.ts/.versionrc.js]
- First found wins
- No merging
- ~~Don't throw if more than 1 found~~ Ideally, throw if more than 1 found
package.json
- [commit-and-tag-version > standard-version]
- Merge with overwriting if both are provided
- Don't throw if both are provided
- Don't throw if conflicting options are provided in both places (???)

(1), (2), and (3) are merged, with duplicate entries in (1) overwriting (2), etc.

Is that about right? What do y'all think about the choice of not throwing in any of these cases - are we providing a foot gun? ~~Should we maybe log a warning on merge?~~

Edit: Updated to reflect recent discussions

TimothyJones · 2023-03-30T05:15:24Z

Unless someone has a good reason for bikeshedding it,

I can only think of good reasons not to bikeshed it 😎

guess we can go with @SharpSeeEr's suggestion.

I agree.

What do y'all think about the choice of not throwing in any of these cases - are we providing a foot gun? Should we maybe log a warning on merge?

I wondered about this too. We do have the advantage that we have clear logging pathway in the console output.

I don't have a strong view on what we should do, but here are my gut feels:

I don't think we need to warn, as #28 is about a case where the user expected to be able to have the same options on the CLI as were specified in the package.json config (and at first glance their setup looked right to me - as would a case where you added one more bumpfile in the CLI arg).

In the case of "this config file is completely ignored because that config file is more important", I think it would be nice to throw - as that would reduce surprise, reduce the possibility of failure, and encourage people to tidy up their repos. I think this ability is a nice to have, and not necessary if it increases complexity too much or is otherwise not wanted.

If someone has a strong opinion that it should behave differently, I'm happy to agree - assuming it is documented and isn't a surprising behaviour.

helmturner · 2023-03-31T02:40:12Z

Sounds all good to me - I've updated that post to reflect the changes. One thing I would add is, for the last line item in #3, I think we should at least warn when there are conflicting options and an override occurs.

I agree about CLI overriding config-file overriding package.json, though. That seems like what I'd expect.

I've done some initial work on the switch to commander, but there's a bit more to this issue than just swapping the CLI parser so it's not much in the grand scheme of things. I'll push a branch when I get a chance to spin it back up.

SharpSeeEr · 2023-04-04T18:41:01Z

I can see providing a warning when two configs contain the same setting, but not when the override comes from the CLI. That may be what you all meant, but just wanted to be sure.

helmturner · 2023-04-06T19:34:21Z

Here's the start of this: #68

SharpSeeEr · 2023-04-12T15:03:18Z

Couple of observations from converting to Typescript:

yargs automatically accepts both the --camelCase and --kebab-case, meaning if you define an option as packageFiles the resulting argv object looks like this:

// --package-files package.json package2.json
{
  packageFiles: ['package.json', 'package2.json'],
  'package-files': ['package.json', 'package2.json'],
}

(@helmturner in #29 (comment):

# lone strings are assumed to be filenames, and filenames delimit array items
$ commit-and-tag-version --packageFiles package.json manifest-beta.json type=json MY_VERSIONS.json updater=my-updater.js name=gradle.properties type=gradle

# or
$ commit-and-tag-version --packagFiles 1=package.json 2=manifest-beta.json 2.type=json 3=MY_VERSIONS.json 3.updater=my-updater.js 4.name=gradle.properties 4.type=gradle

# or, since `type and `updater` are mutually exclusive, we can use a single key and infer it as an updater if it isn't `json`, `gradle`, or `plain-text`
$ commit-and-tag-version --packageFile package.json --packageFile manifest-beta.json json --packageFile MY_VERSIONS.json my-updater.js --packageFile gradle.properties gradle

The three complex options (packageFiles, bumpFiles, types) definitely provide a challenge when it comes to the CLI. These three options are excellent concepts that I think will help us get to the best solution. (@helmturner - please take this as intended, that they truly are great ideas!)

I think the concept of adding additional CLI options is the best way to go.

--package-file package.json --package-file VERSION.md --updater ./md-updater.js

Where the --updater must immediately follow --package-file (or --bump-file).

TimothyJones added the enhancement New feature or request label Jun 25, 2022

TimothyJones mentioned this issue Oct 9, 2022

Unable to use '--bumpFiles' command line arg the same way as the '.versionrc' file #44

Open

This was referenced Mar 15, 2023

[Request] Use .npmrc message key if available #55

Open

Unable to obtain updater for 'manifest-beta.json' #28

Closed

Introduce typescript types (possibly via conversion to typescript) #29

Open

TimothyJones mentioned this issue Mar 25, 2023

CLI runs successfully even when invalid arguments are passed #61

Open

helmturner mentioned this issue Apr 6, 2023

New Config API + Typescript conversion: WIP #68

Closed

helmturner mentioned this issue Apr 6, 2023

Cli update and typescript conversion wip (new branch name) #69

Draft

TimothyJones mentioned this issue Apr 18, 2024

feat(updater): add YAML support (#33, #748) #137

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure all options can be specified via CLI #31

Ensure all options can be specified via CLI #31

TimothyJones commented Jun 25, 2022

helmturner commented Mar 22, 2023 •

edited

Loading

TimothyJones commented Mar 22, 2023

TimothyJones commented Mar 22, 2023

TimothyJones commented Mar 25, 2023

helmturner commented Mar 25, 2023 •

edited

Loading

SharpSeeEr commented Mar 28, 2023

helmturner commented Mar 29, 2023 •

edited

Loading

SharpSeeEr commented Mar 29, 2023

helmturner commented Mar 30, 2023

TimothyJones commented Mar 30, 2023

helmturner commented Mar 30, 2023 •

edited

Loading

TimothyJones commented Mar 30, 2023

helmturner commented Mar 31, 2023

SharpSeeEr commented Apr 4, 2023

helmturner commented Apr 6, 2023

SharpSeeEr commented Apr 12, 2023

Ensure all options can be specified via CLI #31

Ensure all options can be specified via CLI #31

Comments

TimothyJones commented Jun 25, 2022

helmturner commented Mar 22, 2023 • edited Loading

Some thoughts

TimothyJones commented Mar 22, 2023

TimothyJones commented Mar 22, 2023

TimothyJones commented Mar 25, 2023

helmturner commented Mar 25, 2023 • edited Loading

SharpSeeEr commented Mar 28, 2023

helmturner commented Mar 29, 2023 • edited Loading

SharpSeeEr commented Mar 29, 2023

helmturner commented Mar 30, 2023

TimothyJones commented Mar 30, 2023

helmturner commented Mar 30, 2023 • edited Loading

TimothyJones commented Mar 30, 2023

helmturner commented Mar 31, 2023

SharpSeeEr commented Apr 4, 2023

helmturner commented Apr 6, 2023

SharpSeeEr commented Apr 12, 2023

helmturner commented Mar 22, 2023 •

edited

Loading

helmturner commented Mar 25, 2023 •

edited

Loading

helmturner commented Mar 29, 2023 •

edited

Loading

helmturner commented Mar 30, 2023 •

edited

Loading