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

feat(@schematics/angular): add generator for interceptor #15260

Merged
merged 1 commit into from
Sep 26, 2019
Merged

feat(@schematics/angular): add generator for interceptor #15260

merged 1 commit into from
Sep 26, 2019

Conversation

FG-33
Copy link
Contributor

@FG-33 FG-33 commented Aug 6, 2019

It is now possible to generate interceptors using the angular-cli (see #6937):

$ ng g interceptor(ic) test

Result:

CREATE src/app/test.interceptor.spec.ts (417 bytes)
CREATE src/app/test.interceptor.ts (419 bytes)

Is master the correct one to merge into? The documentation says devkit:master but there's no such branch. devkit is too far behind I guess.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@FG-33 FG-33 closed this Aug 6, 2019
@FG-33 FG-33 reopened this Aug 6, 2019
@FG-33 FG-33 changed the base branch from devkit to master August 6, 2019 20:52
@FG-33
Copy link
Contributor Author

FG-33 commented Aug 6, 2019

@googlebot I fixed it.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Aug 6, 2019
@FG-33 FG-33 changed the title feat(@schematic/angular): add generator for interceptor feat(@schematics/angular): add generator for interceptor Aug 7, 2019
@mgechev mgechev added the needs: discussion On the agenda for team meeting to determine next steps label Aug 16, 2019
@vikerman
Copy link
Contributor

Thanks for the PR!

We think both the frequency of adding an interceptor and the complexity of the adding one are pretty low. So we are leaning towards not needing this schematic for now.

@FG-33
Copy link
Contributor Author

FG-33 commented Aug 29, 2019

Thanks for the PR!

We think both the frequency of adding an interceptor and the complexity of the adding one are pretty low. So we are leaning towards not needing this schematic for now.

If you say so. Thanks for the feedback though.

@crutchcorn
Copy link

While in the end of any project, the maintainers are the final deciding voice and that within itself is something I respect greatly, it seems to me (from a user looking at the project) that many of the schematics are simply convinience tools.

EG, the component generator. The component complexity seems relatively low, especially once you have an initial component to copy+paste off of. However, for an interceptor I don't often use them for various projects and then have to Google the correct usage of it (or find which of my many Angular projects has one to copy off of), so having this generator (even if the complexity is low overall) is a big convinience for me as I end up using it enough to come looking for this issue every now and then

The engagement with the linked issue (which has unfortunately fallen victim to the "+1" comment wave, but even with reacts alone) seems to show a further community interest

Just giving my own thoughts, huge thank you to everyone on the Angular team for your persistent hard-work on the project regardless of where this issue lands

@vikerman vikerman added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed needs: discussion On the agenda for team meeting to determine next steps labels Sep 12, 2019
@vikerman
Copy link
Contributor

vikerman commented Sep 12, 2019

Hi @FG-33 - We discussed this again and decided to accept this PR!

We had some comments on the PR(forthcoming) and we can merge this as soon as they are addressed. Thanks!

@clydin
Copy link
Member

clydin commented Sep 12, 2019

Also, to answer your question, master is the correct branch.
Can you rebase on top of the latest master as well once the changes are addressed?

@clydin clydin added the target: major This PR is targeted for the next major release label Sep 12, 2019
@clydin clydin removed the request for review from filipesilva September 12, 2019 17:51
@alan-agius4
Copy link
Collaborator

Can you also add interceptor in

['class', 'component', 'directive', 'guard', 'module', 'pipe', 'service'].forEach((type) => {

@FG-33
Copy link
Contributor Author

FG-33 commented Sep 26, 2019

@clydin I addressed all the mentioned changes. If anything's missing, let me know.

@clydin clydin removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Sep 26, 2019
@vikerman vikerman merged commit 9de282c into angular:master Sep 26, 2019
@FG-33 FG-33 deleted the interceptor-generator branch September 26, 2019 17:09
@simonua
Copy link
Contributor

simonua commented Oct 3, 2019

I am glad this got merged in. Echoing @crutchcorn's appreciation for the Angular team as well as his reasoning for having a generator, we have several teams in my organization that are starting to develop with Angular 8 now and having a generator in place not only for the meat and potatoes of Angular but also these arguably more tangential use cases is helpful as I can simply mention the generator syntax in our guidelines and documentation and take more comfort in code being generated uniformly across teams. This increased conformance then cuts out nuisance PR comments that may be the result of copy/paste code propagation.

Long story short, thank you! It is appreciated.

@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 Nov 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants