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

fix: retain enable json flag get/set apply flag at parse #460

Merged
merged 2 commits into from
Aug 8, 2022

Conversation

peternhale
Copy link
Contributor

No description provided.

@peternhale peternhale requested a review from mdonnalley August 8, 2022 15:22
src/command.ts Outdated
@@ -246,7 +246,7 @@ export default abstract class Command {
if (!options) options = this.constructor as any
const opts = {context: this, ...options}
// the spread operator doesn't work with getters so we have to manually add it here
opts.flags = options?.flags
opts.flags = (this.ctor.enableJsonFlag ? {...options?.flags, ...jsonFlag} : options?.flags) as Interfaces.FlagInput<F>
Copy link
Contributor

@mdonnalley mdonnalley Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to also change enableJsonFlag from a getter/setter to static property. Otherwise, it'll still allow the --json flag when a subclass has set enableJsonFlag to false

with getter/setter:

~/code/global-flags » bin/dev status --json
undefined

with static property

~/code/global-flags » bin/dev status --json
Error: Unexpected argument: --json
See more help with --help
    at validateArgs (/Users/mdonnalley/repos/oclif/core/lib/parser/validate.js:11:19)
    at Object.validate (/Users/mdonnalley/repos/oclif/core/lib/parser/validate.js:78:5)
    at Object.parse (/Users/mdonnalley/repos/oclif/core/lib/parser/index.js:30:7)
    at async Status.run (/Users/mdonnalley/code/global-flags/src/commands/status.ts:82:20)
    at async Status._run (/Users/mdonnalley/repos/oclif/core/lib/command.js:65:22)
    at async Config.runCommand (/Users/mdonnalley/repos/oclif/core/lib/config/config.js:270:25)
    at async Object.run (/Users/mdonnalley/repos/oclif/core/lib/main.js:76:5)

Never mind. As I was testing this I realized that deferring the json inclusion to parse means that help isn't ever aware of the json flag... which is why the tests were failing when you tried the solution I suggested.

So now that I know that, I think your original solution might be the best path forward

  static set enableJsonFlag(value: boolean) {
    this._enableJsonFlag = value
    if (value === true) {
      this.globalFlags = jsonFlag
    } else {
      // @ts-ignore
      delete this.globalFlags.json
      // @ts-ignore
      delete this.flags.json
    }
  }

Sorry for being difficult!

@peternhale peternhale requested a review from mdonnalley August 8, 2022 16:00
@mdonnalley
Copy link
Contributor

QA

Command class with enableJsonFlag = true
✅ run command with --help, shows --json
✅ run command with --json, succeeds
✅ order of enableJsonFlag and flags does not matter

Subclass extends Parent Command class with enableJsonFlag = true
✅ run command with --help, shows --json
✅ run command with --json, succeeds
✅ order of enableJsonFlag and flags does not matter

Subclass with enableJsonFlag = false extends Parent Command class with enableJsonFlag = true
✅ run command with --help, does not show --json
✅ run command with --json, fails
✅ order of enableJsonFlag and flags does not matter

@mdonnalley mdonnalley merged commit 9812937 into main Aug 8, 2022
@mdonnalley mdonnalley deleted the phale/enable-json-false branch August 8, 2022 16:13
mdonnalley added a commit that referenced this pull request Aug 9, 2022
@mdonnalley mdonnalley mentioned this pull request Aug 9, 2022
mdonnalley added a commit that referenced this pull request Aug 9, 2022
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.

2 participants