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

feat(module): component optional when generating module #3389

Merged
merged 25 commits into from
Dec 9, 2016
Merged

feat(module): component optional when generating module #3389

merged 25 commits into from
Dec 9, 2016

Conversation

baruchvlz
Copy link
Contributor

@baruchvlz baruchvlz commented Dec 5, 2016 •
edited
Loading

As mentioned in #3365

Usage: ng g m foo --component

@baruchvlz
Copy link
Contributor Author

Noob me forgot about e2e tests. I will fix and update tomorrow.

@baruchvlz
Copy link
Contributor Author

baruchvlz commented Dec 5, 2016 •
edited
Loading

@filipesilva Sorry if this is out of line, but I was just wondering if there's a way to trigger the Travis build without having to commit anything. It seems the build cancelled when I pushed the last commit.

@hansl
Copy link
Contributor

hansl commented Dec 7, 2016

We're discussing this internally to see if that's the way we want to move forward. The other solution would be to have components generated by default and have a --no-component flag (or --component=false).

@baruchvlz
Copy link
Contributor Author

baruchvlz commented Dec 7, 2016 •
edited
Loading

If you guys choose to go for the second option I can change the code to fit the need. Thanks for the update.

@hansl
Copy link
Contributor

hansl commented Dec 7, 2016

Okay we're going with this PR as is.

@hansl
Copy link
Contributor

hansl commented Dec 7, 2016

Could you add a simple e2e test that actually call your flag? I want to make sure we avoid regressions.

@hansl
Copy link
Contributor

hansl commented Dec 7, 2016

Other than adding a test, this LGTM.

@baruchvlz
Copy link
Contributor Author

baruchvlz commented Dec 8, 2016 •
edited
Loading

@hansl Correct me if I'm wrong, but the --routing has a e2e test here with it's new functionality. Also, the module default behavior is being tested here, also with the new behavior.

@hansl
Copy link
Contributor

hansl commented Dec 8, 2016

@baruchvlz yes, but I want a test that actually uses --component. There's no testing on using the flag itself, which is the regression I want to avoid (say we get rid of the flag entirely later on).

@baruchvlz
Copy link
Contributor Author

The --component flag itself doesn't exist in the current code base, so I don't think there's any regression. I added the --component flag based on the suggestion in the issue and then removed it when it was decided to go a different direction, but the flag was never introduced into the code.

Sorry if I'm misunderstanding, I don't have experience working with such large projects.

@hansl
Copy link
Contributor

hansl commented Dec 9, 2016

No it's okay. We'll add a --component flag if/when people request it. LGTM.

@hansl hansl merged commit 2fb2d13 into angular:master Dec 9, 2016
@hansl hansl mentioned this pull request Dec 9, 2016
Closed
@Meligy
Copy link
Contributor

Meligy commented Jan 9, 2017

I think this default is a bit confusing.

Example: #3255 (comment)

@Meligy Meligy mentioned this pull request Jan 9, 2017
Closed
MRHarrison pushed a commit to MRHarrison/angular-cli that referenced this pull request Feb 9, 2017
@baruchvlz @MRHarrison
feat(module): component optional when generating module (angular#3389)
ead6349 
* change URL query key name

* generate component with module made optional

* revert changes done to module cause open PR

* components when generating module is optional

* tests updated for component flag

* fix parent/child compnent test to fit new flag

* updated test for parent/child module generate

* removed component testing from basic module e2e

* adjusted e2e test for generate module

* ng g m creates module only with routing flag

* unit test for new generate module behavior

* e2e tests for g m new behavior

* remove spec test

* shorten test module name to keep 100 char limit
@rageshvr rageshvr mentioned this pull request Jun 29, 2017
Closed
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@hansl hansl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@baruchvlz @hansl @Meligy @filipesilva @googlebot