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.

Sign up for GitHub

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

Jump to bottom

Refactor(schematics) use default project if project is not provided #1598

Merged
merged 7 commits into from
Mar 8, 2019
Merged

Refactor(schematics) use default project if project is not provided #1598

merged 7 commits into from
Mar 8, 2019

Conversation

santoshyadavdev
Copy link
Sponsor Contributor

@santoshyadavdev santoshyadavdev commented Mar 3, 2019 •
edited
Loading

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?
Take defaultProject in case project flag is not provided.

[ ] Bugfix
[ ] 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?

First project from project array is considered when project flag is not provided.

Closes #1564

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@coveralls
Copy link

coveralls commented Mar 3, 2019 •
edited
Loading

Coverage Status

Coverage decreased (-0.004%) to 89.509% when pulling 69f08c7 on santoshyadav198613:refactor(Schematics)-use-defaultProject-if-project-is-not-provided into 5cb9a46 on ngrx:master.

@ngrxbot
Copy link
Collaborator

ngrxbot commented Mar 3, 2019

Preview docs changes for d234c09 at https://previews.ngrx.io/pr1598-d234c09/

timdeschryver
timdeschryver requested changes Mar 3, 2019
View reviewed changes
Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

Could you also write a test for this change?

modules/effects/schematics-core/utility/project.ts Outdated
@@ -13,7 +13,10 @@ export function getProject(
const workspace = getWorkspace(host);

if (!options.project) {
options.project = Object.keys(workspace.projects)[0];
options.project =
workspace.defaultProject != undefined
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
workspace.defaultProject != undefined
workspace.defaultProject !== undefined

@santoshyadavdev
Copy link
Sponsor Contributor Author

Could you also write a test for this change?

This is draft PR, will let you know once i complete it.

@ngrxbot
Copy link
Collaborator

ngrxbot commented Mar 4, 2019

Preview docs changes for cc3aeca at https://previews.ngrx.io/pr1598-cc3aeca/

@ngrxbot
Copy link
Collaborator

ngrxbot commented Mar 4, 2019

Preview docs changes for c5204d3 at https://previews.ngrx.io/pr1598-c5204d3/

brandonroberts
brandonroberts requested changes Mar 4, 2019
View reviewed changes
modules/schematics/src/store/index.spec.ts Outdated
@@ -18,7 +18,7 @@ describe('Store Schematic', () => {
);
const defaultOptions: StoreOptions = {
name: 'foo',
project: 'bar',
Copy link
Member

Choose a reason for hiding this comment

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

Don't modify existing defaults. Add an additional test that provides an empty project

@ngrxbot
Copy link
Collaborator

ngrxbot commented Mar 4, 2019

Preview docs changes for 69f08c7 at https://previews.ngrx.io/pr1598-69f08c7/

@santoshyadavdev
Merge branch 'master' of https://github.com/ngrx/platform into refact…
9fc7fe6 
…or(Schematics)-use-defaultProject-if-project-is-not-provided
@ngrxbot
Copy link
Collaborator

ngrxbot commented Mar 5, 2019

Preview docs changes for 9fc7fe6 at https://previews.ngrx.io/pr1598-9fc7fe6/

@brandonroberts
Copy link
Member

LGTM. @timdeschryver ?

timdeschryver
timdeschryver approved these changes Mar 7, 2019
View reviewed changes
Copy link
Member

@timdeschryver timdeschryver left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Also LGTM 👍

@brandonroberts
Copy link
Member

@santoshyadav198613 if you'll mark this a ready for review, we'll get it merged.

@santoshyadavdev santoshyadavdev marked this pull request as ready for review March 8, 2019 05:33
@santoshyadavdev
Copy link
Sponsor Contributor Author

@santoshyadav198613 if you'll mark this a ready for review, we'll get it merged.

Done.

@brandonroberts brandonroberts merged commit 3d24787 into ngrx:master Mar 8, 2019
tja4472 pushed a commit to tja4472/platform that referenced this pull request Mar 20, 2019
@santoshyadavdev @tja4472
refactor(schematics) use default project if project is not provided (n…
1a7a2e6 
…grx#1598)

Closes ngrx#1564
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brandonroberts brandonroberts brandonroberts approved these changes

@timdeschryver timdeschryver timdeschryver approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Schematics: use defaultProject if project is not provided
5 participants
@santoshyadavdev @coveralls @ngrxbot @brandonroberts @timdeschryver