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(schematics): fix typo in option name #2522

Merged
merged 5 commits into from
May 27, 2020

Conversation

aszechlicki
Copy link
Contributor

@aszechlicki aszechlicki commented May 11, 2020

rename skipTests option in action schematics to skipTest, consistent with naming in other schematics
pass option value instead of hardcoded true in feature schematics
update tests

fixes #2521

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Closes #2521

What is the new behavior?

Action schematics skipTests option now has consistent name with other schematics - skipTest
feature schematics now passes actual option value instead of hardcoded true to skipTest option of action schematics

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@ngrxbot
Copy link
Collaborator

ngrxbot commented May 11, 2020

Preview docs changes for 54c0af3 at https://previews.ngrx.io/pr2522-54c0af3/

@brandonroberts
Copy link
Member

@aszechlicki I misread this. We want skipTests to be consistent across the board, which is inline with what the upstream schematics are using:

https://github.com/angular/angular-cli/blob/master/packages/schematics/angular/component/schema.json#L96

@aszechlicki
Copy link
Contributor Author

@brandonroberts so I should rename all skipTest to skipTests?

@brandonroberts
Copy link
Member

Yes

@brandonroberts
Copy link
Member

Sorry. We should probably keep both for now, deprecate skipTest, and remove it in v10.

@aszechlicki
Copy link
Contributor Author

ok, then can you tell me how to mark this property as deprecated?
I assume JsDoc in index.ts, but what is the convention in schema.json and .md files?

@brandonroberts
Copy link
Member

In the schema.json, just add Deprecated to the description, and just replace the deprecated version in the markdown.

@aszechlicki aszechlicki force-pushed the schematics-skip-test branch from 6b0ec3c to 7766a7f Compare May 12, 2020 22:58
@@ -27,6 +27,11 @@
"description": "Flag to indicate if a dir is created."
},
"skipTest": {
"type": "boolean",
"description": "(Deprecated) Use skipTests instead",
Copy link
Member

Choose a reason for hiding this comment

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

Fyi, instead of adding deprecated to the description we could use the x-deprecated property.
This way the cli will log a warning when skipTest is being used.

As an example, see the entryComponent option

Copy link
Member

Choose a reason for hiding this comment

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

Still needs x-deprecated

Arkadiusz Szechlicki added 3 commits May 21, 2020 20:34
rename skipTests option in action schematics to skipTest, consistent with naming in other schematics
pass option value instead of hardcoded true in feature schematics
update tests

fixes ngrx#2521
make sure old option still works
@aszechlicki aszechlicki force-pushed the schematics-skip-test branch from 7766a7f to 9d1853a Compare May 21, 2020 18:38
Copy link
Member

@brandonroberts brandonroberts left a comment

Choose a reason for hiding this comment

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

Couple more deprecations. Everything else LGTM

@@ -27,6 +27,11 @@
"description": "Flag to indicate if a dir is created."
},
"skipTest": {
"type": "boolean",
"description": "(Deprecated) Use skipTests instead",
Copy link
Member

Choose a reason for hiding this comment

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

Still needs x-deprecated

@@ -25,6 +25,11 @@
"aliases": ["p"]
},
"skipTest": {
"type": "boolean",
"description": "(Deprecated) Use skipTests instead",
"default": false
Copy link
Member

Choose a reason for hiding this comment

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

Still needs x-deprecated

@brandonroberts brandonroberts merged commit 83033d7 into ngrx:master May 27, 2020
@brandonroberts
Copy link
Member

Thanks @aszechlicki!

BioPhoton pushed a commit to BioPhoton/platform that referenced this pull request Jun 5, 2020
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.

schematics:feature --skipTest flag generates actions.spec.ts
4 participants