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

Wr/spread base flags #441

Merged
merged 7 commits into from
Jul 22, 2024
Merged

Conversation

WillieRuemmele
Copy link
Contributor

@WillieRuemmele WillieRuemmele commented Jul 16, 2024

@W-15837411@

warning to encourage inheriting from SfCommand directly

/Users/william.ruemmele/projects/oss/plugin-devops-center/src/commands/project/deploy/pipeline/report.ts
  15:22  warning  In order to inherit default flags correctly, extend from SfCommand directly                          sf-plugin/only-extend-SfCommand

error when not spreading parent class' flags

  20:3   error            When not directly extending SfCommand, the parent's flags must be spread like flags = {...ReportOnPromoteCommand.flags}
  sf-plugin/spread-base-flags

warn for these rules until they can be fixed in the offending plugins
https://github.com/salesforcecli/eslint-plugin-sf-plugin/actions/runs/9976522635/job/27569016725?pr=441

@WillieRuemmele WillieRuemmele changed the base branch from main to wr/noDefaultAndDependsOn July 16, 2024 22:25
@cristiand391
Copy link
Member

cristiand391 commented Jul 22, 2024

QA notes:

setup:
build and installed eslint plugin as mentioned in the contributing doc, tested on plugin-data

sf-plugin/only-extend-SfCommand

✅ warn if not extending SfCommand directly
✅ only checks TS files in src/commands

sf-plugin/spread-base-flags

I was reading the WI again and I think it was meant to cover SfCommand.baseFlags being lost when extending SfCommand, but when I try that I always get a TS error if I don't spread SfCommand.baseFlags so I wonder if this rule needs to cover that.
https://github.com/salesforcecli/sf-plugins-core/blob/7f7fe9bab0896fd6a7f7414cb663226e60c6977c/src/sfCommand.ts#L123

Screenshot 2024-07-22 at 12 11 16 PM

Screenshot 2024-07-22 at 12 20 28 PM

✅ warn if extending SfCommand and not spreading SfCommand.flags
SfCommand.flags isn't defined so commands extending SfCommand aren't missing any flags right now, do we need to keep this rule?
🟡 no autofix, is there a scenario where autofixing this would be bad?

@cristiand391
Copy link
Member

sf-plugin/spread-base-flags

✅ cmd extends from SfCommand directly, no warns
✅ cmd extends dummy SfCommand wrapper, gets warn to spread Wrapper.flags:

abstract class Wrapper extends SfCommand<LalaResult> {}
export default class Lala extends Wrapper {
    ...
}

✅ same scenario from above but define baseFlags in Lala class, gets warn to spread Wrapper.baseFlags.
✅ cmd without flags or baseFlags doesn't get flagged by rule

@cristiand391 cristiand391 merged commit aa79026 into wr/noDefaultAndDependsOn Jul 22, 2024
44 checks passed
@cristiand391 cristiand391 deleted the wr/spreadBaseFlags branch July 22, 2024 19:56
cristiand391 pushed a commit that referenced this pull request Jul 22, 2024
…nds-on-flags` rules (#440)

* feat: add no default and depends on flag configuration rule

* chore: edge case

* chore: fix circular error

* docs: generate docs

* chore: fix [0]

* chore: fix [0] (2)

* Wr/spread base flags (#441)

* feat: add only extend from SfCommand

* feat: add spread super command flags prop

* chore: update messages to include super class name

* chore: identify SfCommand parent class

* chore: move from error to warn on rollout

* chore: ..flags, ...baseFlags

* chore: update comment
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