refactor(schematics):entity schematics should update the state to plural #1596

santoshyadavdev · 2019-03-03T14:08:04Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/ngrx/platform/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Documentation has been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

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

What is the current behavior?

currently the state variable is singular updating the same to plural
Closes #1412

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

coveralls · 2019-03-03T14:15:02Z

Coverage increased (+0.02%) to 89.529% when pulling 2ceba07 on santoshyadav198613:refactor(schematics)-Entity-schematics-should-update-the-state-with-a-plural-instead-of-a-singular into 5cb9a46 on ngrx:master.

ngrxbot · 2019-03-03T14:16:05Z

Preview docs changes for 46c246b at https://previews.ngrx.io/pr1596-46c246b/

ngrxbot · 2019-03-03T14:22:24Z

Preview docs changes for dbde8bc at https://previews.ngrx.io/pr1596-dbde8bc/

santoshyadavdev · 2019-03-03T14:28:45Z

@brandonroberts ,

Need help here don't know why CI is getting failed for e2e testing.

ngrxbot · 2019-03-03T14:30:26Z

Preview docs changes for 2ceba07 at https://previews.ngrx.io/pr1596-2ceba07/

timdeschryver · 2019-03-03T21:25:22Z

@santoshyadav198613 , the failing e2e test isn't related to your change.
I will take a look at it.

timdeschryver · 2019-03-03T21:33:25Z

The problem is that we are caching the cypress installation in the wrong build step.
This should be solved when #1593 gets merged.

brandonroberts

This should be isolated to @ngrx/entity for now. And it needs some tests for common scenarios.

santoshyadavdev · 2019-03-04T17:02:42Z

d it needs some tests for common scenarios.

But this code is part of common function inside schematics-core , can you guide me how should we handle it?

brandonroberts · 2019-03-04T18:12:19Z

You create a test for a generated schematic and check that the output is what you expect. For example, your test would use the entity schematic to generate a user collection. The schematic should generate a property named users, with the associated files.

timdeschryver

Should we use the pluralize method at this point?
Because I'm a bit afraid this would lead to inconsistencies, for example, while scaffolding the effects we append the name with an s.

santoshyadavdev · 2019-03-04T19:26:16Z

Should we use the pluralize method at this point?
Because I'm a bit afraid this would lead to inconsistencies, for example, while scaffolding the effects we append the name with an s.

Ok let me check it.

santoshyadavdev · 2019-03-04T19:26:42Z

You create a test for a generated schematic and check that the output is what you expect. For example, your test would use the entity schematic to generate a user collection. The schematic should generate a property named users, with the associated files.

ok will update and let you know.

…or(schematics)-Entity-schematics-should-update-the-state-with-a-plural-instead-of-a-singular

santoshyadavdev · 2019-03-05T18:28:29Z

You create a test for a generated schematic and check that the output is what you expect. For example, your test would use the entity schematic to generate a user collection. The schematic should generate a property named users, with the associated files.

Hi @brandonroberts ,

Added a test case for the scenario this is what i have done.
Added one store and entity and then verified if state variable is plural or not same for reducers.

santoshyadavdev · 2019-03-05T18:29:32Z

@brandonroberts and @timdeschryver let me know about the usage of plural function should i remove it.

ngrxbot · 2019-03-05T18:30:18Z

Preview docs changes for ca25fe4 at https://previews.ngrx.io/pr1596-ca25fe4/

santoshyadavdev · 2019-03-06T17:18:54Z

Hi @brandonroberts ,
Can you verify again, wrote the test case as per our discussion.

brandonroberts · 2019-03-07T21:08:12Z

@santoshyadav198613 we don't want to modify existing behavior for other areas. Instead, add an options object to the ngrx-utils methods as the second argument that's set to { pluralize: false }. For entity schematics, pass { pluralize: true }and use it whether to callpluralizeor justcamelize` inside those functions.

timdeschryver · 2019-03-07T21:41:09Z

I'm a bit hesitant to add pluralize (for now) because this would lead to inconsistencies, e.g. city would be pluralized to cities, and in the other schematics to citys.

What about keeping this PR scoped to making the entities pluralized (adding an s), and to create a separate issue to pluralize all the schematics via this function?

brandonroberts · 2019-03-07T21:43:55Z

@timdeschryver yes, scoping this to entities is what I had in mind also.

santoshyadavdev · 2019-03-08T05:35:27Z

@santoshyadav198613 we don't want to modify existing behavior for other areas. Instead, add an options object to the ngrx-utils methods as the second argument that's set to { pluralize: false }. For entity schematics, pass { pluralize: true }and use it whether to callpluralizeor justcamelize` inside those functions.

Thanks @brandonroberts ,
Will make the changes.

…or(schematics)-Entity-schematics-should-update-the-state-with-a-plural-instead-of-a-singular

ngrxbot · 2019-03-08T17:07:24Z

Preview docs changes for ed6655d at https://previews.ngrx.io/pr1596-ed6655d/

ngrxbot · 2019-03-08T17:12:27Z

Preview docs changes for 1510211 at https://previews.ngrx.io/pr1596-1510211/

ngrxbot · 2019-03-08T17:28:28Z

Preview docs changes for 757d04e at https://previews.ngrx.io/pr1596-757d04e/

santoshyadavdev · 2019-03-08T17:36:26Z

Hi @brandonroberts and @timdeschryver ,
Done with the changes as per discussion, test cases are also added/modified.
The change is restricted to entity only, also appending only 's', keeping pluralize let me know if i should remove it.

modules/schematics/src/entity/index.spec.ts

modules/schematics-core/utility/ngrx-utils.ts

modules/schematics/src/entity/schema.json

modules/schematics/src/entity/schema.ts

ngrxbot · 2019-03-09T11:07:12Z

Preview docs changes for 0b4a3a2 at https://previews.ngrx.io/pr1596-0b4a3a2/

brandonroberts

Pretty close. Few more changes

brandonroberts · 2019-03-09T18:23:10Z

modules/schematics-core/utility/ngrx-utils.ts

@@ -90,11 +90,13 @@ export function addReducerToStateInterface(
    return new NoopChange();
  }

+  let state = stringUtils.camelize(options.name);


Suggested change

let state = stringUtils.camelize(options.name);

const state = options.plural ? stringUtils.pluralize(options.name) : stringUtils.camelize(options.name);

Let's use the pluralize method instead of tacking on an s.

brandonroberts · 2019-03-09T18:25:10Z

modules/schematics-core/utility/strings.ts

+ 'innerHTML'.pluralize()         // 'InnerHTMLs'
+ 'action_name'.pluralize()       // 'actionNames'
+ 'css-class-name'.pluralize()    // 'cssClassNames'
+ 'regex'.capitalize()            // 'regexes'


Suggested change

'regex'.capitalize() // 'regexes'

'regex'.pluralize() // 'regexes'

brandonroberts · 2019-03-09T18:25:40Z

modules/schematics-core/utility/strings.ts

+ 'action_name'.pluralize()       // 'actionNames'
+ 'css-class-name'.pluralize()    // 'cssClassNames'
+ 'regex'.capitalize()            // 'regexes'
+ 'user'.capitalize()             // 'users'


Suggested change

'user'.capitalize() // 'users'

'user'.pluralize() // 'users'

brandonroberts · 2019-03-09T18:32:48Z

modules/schematics/schematics-core/utility/strings.ts

@@ -110,6 +110,25 @@ export function capitalize(str: string): string {
  return str.charAt(0).toUpperCase() + str.substr(1);
 }

+/**
+ Returns the Pluralize form of a string


Suggested change

Returns the Pluralize form of a string

Returns the plural form of a string

ngrxbot · 2019-03-10T14:56:08Z

Preview docs changes for 4eb91da at https://previews.ngrx.io/pr1596-4eb91da/

santoshyadavdev · 2019-03-11T18:56:37Z

Hi @brandonroberts @timdeschryver ,
All changes are done please review.

timdeschryver

LGTM

…1596) Closes ngrx#1412

santoshyadavdev added 2 commits March 3, 2019 19:29

refactor(schematics):entity schematics should update the state to plural

46c246b

changes for reducer

dbde8bc

comments corrected

2ceba07

brandonroberts requested changes Mar 4, 2019

View reviewed changes

timdeschryver reviewed Mar 4, 2019

View reviewed changes

santoshyadavdev added 2 commits March 5, 2019 22:57

Merge branch 'master' of https://github.com/ngrx/platform into refact…

166eabe

…or(schematics)-Entity-schematics-should-update-the-state-with-a-plural-instead-of-a-singular

added test case

ca25fe4

brandonroberts added Project: Schematics Needs Cleanup Review changes needed labels Mar 7, 2019

santoshyadavdev added 2 commits March 8, 2019 22:25

changes to restrict plural to entity

672f603

removed console

ed6655d

Merge branch 'master' of https://github.com/ngrx/platform into refact…

1510211

…or(schematics)-Entity-schematics-should-update-the-state-with-a-plural-instead-of-a-singular

removed .bazel files

757d04e

timdeschryver requested changes Mar 8, 2019

View reviewed changes

code review comments

0b4a3a2

brandonroberts requested changes Mar 9, 2019

View reviewed changes

code review changes

4eb91da

timdeschryver approved these changes Mar 11, 2019

View reviewed changes

brandonroberts approved these changes Mar 12, 2019

View reviewed changes

brandonroberts merged commit 1e49530 into ngrx:master Mar 12, 2019

tja4472 pushed a commit to tja4472/platform that referenced this pull request Mar 20, 2019

feat(schematics): use plural for entity schematics reducer key (ngrx#…

a9ca575

…1596) Closes ngrx#1412

	let state = stringUtils.camelize(options.name);
	const state = options.plural ? stringUtils.pluralize(options.name) : stringUtils.camelize(options.name);

	'regex'.capitalize() // 'regexes'
	'regex'.pluralize() // 'regexes'

	Returns the Pluralize form of a string
	Returns the plural form of a string

refactor(schematics):entity schematics should update the state to plural #1596

refactor(schematics):entity schematics should update the state to plural #1596

Conversation

santoshyadavdev commented Mar 3, 2019 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

coveralls commented Mar 3, 2019 • edited Loading

ngrxbot commented Mar 3, 2019

ngrxbot commented Mar 3, 2019

santoshyadavdev commented Mar 3, 2019

ngrxbot commented Mar 3, 2019

timdeschryver commented Mar 3, 2019

timdeschryver commented Mar 3, 2019

brandonroberts left a comment

Choose a reason for hiding this comment

santoshyadavdev commented Mar 4, 2019

brandonroberts commented Mar 4, 2019

timdeschryver left a comment

Choose a reason for hiding this comment

santoshyadavdev commented Mar 4, 2019

santoshyadavdev commented Mar 4, 2019

santoshyadavdev commented Mar 5, 2019

santoshyadavdev commented Mar 5, 2019

ngrxbot commented Mar 5, 2019

santoshyadavdev commented Mar 6, 2019

brandonroberts commented Mar 7, 2019

timdeschryver commented Mar 7, 2019

brandonroberts commented Mar 7, 2019

santoshyadavdev commented Mar 8, 2019

ngrxbot commented Mar 8, 2019

ngrxbot commented Mar 8, 2019

ngrxbot commented Mar 8, 2019

santoshyadavdev commented Mar 8, 2019

ngrxbot commented Mar 9, 2019

brandonroberts left a comment

Choose a reason for hiding this comment

brandonroberts Mar 9, 2019 • edited Loading

Choose a reason for hiding this comment

santoshyadavdev Mar 10, 2019

Choose a reason for hiding this comment

brandonroberts Mar 9, 2019

Choose a reason for hiding this comment

santoshyadavdev Mar 10, 2019

Choose a reason for hiding this comment

brandonroberts Mar 9, 2019

Choose a reason for hiding this comment

santoshyadavdev Mar 10, 2019

Choose a reason for hiding this comment

brandonroberts Mar 9, 2019

Choose a reason for hiding this comment

santoshyadavdev Mar 10, 2019

Choose a reason for hiding this comment

ngrxbot commented Mar 10, 2019

santoshyadavdev commented Mar 11, 2019

timdeschryver left a comment

Choose a reason for hiding this comment

santoshyadavdev commented Mar 3, 2019 •

edited

Loading

coveralls commented Mar 3, 2019 •

edited

Loading

brandonroberts Mar 9, 2019 •

edited

Loading