build(docs-infra): support hiding constructors of injectable classes #24529

petebacondarwin · 2018-06-14T22:54:59Z

Class that are injectable often have constructors that should not be
called by the application developer. It is the responsibility of the
injector to instantiate the class and the constructor often contains
private implementation details that may need to change.

mary-poppins · 2018-06-14T23:02:10Z

You can preview aca3297 at https://pr24529-aca3297.ngbuilds.io/.

gkalpak

You might want to also update overview-dump.template.html.

gkalpak · 2018-06-15T09:04:23Z

On second thought, I find the tag name a little confusing. docsNotRequired might also mean "you don't need to add docs" (for whatever reason 😛). It is not clear that it will be hidden in docs.

Maybe something more assertive, like hideInDocs or excludeFromDocs?

petebacondarwin · 2018-06-15T09:25:22Z

How about even more descriptive and specific: @injectableConstructor?
Or... we could automatically hide the constructor if the following is true:

the class has @Injectable decorator
the constructor has no description

What do you think?

gkalpak · 2018-06-15T10:33:35Z

If it is true that all @Injectable constructors should not appear in docs, then option (2) sounds perfect 😃

Classes that are injectable often have constructors that should not be called by the application developer. It is the responsibility of the injector to instantiate the class and the constructor often contains private implementation details that may need to change. This commit removes constructors from the docs for API items that are both: a) Marked with an injectable decorator (e.g. Injectable, Directive, Component, Pipe), and b) Have no constructor description This second rule allows the developer to override the removal if there is something useful to say about the constructor. Note that "normal" classes such as `angimations/HttpHeaders` do not have their constructor removed, despite (at this time) having no description.

petebacondarwin · 2018-06-15T12:10:12Z

I rewrote this PR so you don't need to annotate the class constructor to make it hidden. I also noticed that Component and other decorators also indicate that the class is injectable so I am removing their decorators too. See the commit message for details of the approach.

petebacondarwin · 2018-06-15T12:13:17Z

@gkalpak PTAL due to the complete rewrite

gkalpak · 2018-06-15T13:05:33Z

aio/tools/transforms/angular-api-package/processors/removeInjectableConstructors.spec.js

+    expect(processor.$runBefore).toEqual(['docs-processed']);
+  });
+
+  it('should remove undocumented constructors from docs that have an "Injectable" decorator on it', () => {


it --> them 😇

gkalpak · 2018-06-15T13:07:35Z

aio/tools/transforms/angular-api-package/processors/removeInjectableConstructors.spec.js

+    const injector = dgeni.configureInjector();
+    const processor = injector.get('removeInjectableConstructors');
+    expect(processor.$process).toBeDefined();
+    expect(processor.$runAfter).toEqual(['processing-docs']);


Shouldn't this be ['processing-docs', 'splitDescription']?

gkalpak · 2018-06-15T13:08:52Z

aio/tools/transforms/angular-api-package/processors/removeInjectableConstructors.js

+  return {
+    $runAfter: ['processing-docs', 'splitDescription'],
+    $runBefore: ['docs-processed'],
+    injectableDecorators: ['Injectable', 'Directive', 'Component', 'Pipe'],


What about NgModule?

gkalpak · 2018-06-15T13:10:10Z

aio/tools/transforms/angular-api-package/processors/removeInjectableConstructors.js

+      docs.forEach(doc => {
+        if (
+          doc.constructorDoc &&
+          !doc.constructorDoc.shortDescription &&


Can a constructorDoc have a description but no shortDescription?

Not if it runs after the splitDescription processor.

jbogarthyde · 2018-06-21T19:49:40Z

This solution makes the doc writer responsible for NOT including a doc comment (or including an empty one) on the constructor for a service. Is that what you want? If so, I will add it to the guidelines.

An explicit tag would have the advantage of showing that the choice not to document is explicit, and not a side-effect of incomplete documentation.

petebacondarwin · 2018-06-22T10:40:36Z

Actually they could still add an explicit tag, e.g. @docsNotRequired, which would have no effect but at least it would be a marker to the reviewer? This tag could even have a boilerplate comment. For example:

@docsNotRequired this class is `Injectable` so the constructor is a private implementation.

As it stands we would not fail if this were missing but perhaps it could become enforceable later.

…lasses

mary-poppins · 2018-06-22T21:01:22Z

You can preview a4fed6c at https://pr24529-a4fed6c.ngbuilds.io/.

jbogarthyde · 2018-06-25T14:57:59Z

I like that with the boilerplate -- it would make people think about what they are doing, and provide an explanation for users who are looking at the code.

…lasses

mary-poppins · 2018-06-26T13:26:08Z

You can preview 4feac28 at https://pr24529-4feac28.ngbuilds.io/.

…24529) Classes that are injectable often have constructors that should not be called by the application developer. It is the responsibility of the injector to instantiate the class and the constructor often contains private implementation details that may need to change. This commit removes constructors from the docs for API items that are both: a) Marked with an injectable decorator (e.g. Injectable, Directive, Component, Pipe), and b) Have no constructor description This second rule allows the developer to override the removal if there is something useful to say about the constructor. Note that "normal" classes such as `angimations/HttpHeaders` do not have their constructor removed, despite (at this time) having no description. PR Close #24529

angular-automatic-lock-bot · 2019-09-13T20:21:23Z

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.}

googlebot added the cla: yes label Jun 14, 2018

petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer effort1: hours severity1: confusing target: patch This PR is targeted for the next patch release labels Jun 14, 2018

petebacondarwin added this to the Backlog milestone Jun 14, 2018

ngbot bot modified the milestones: Backlog, needsTriage Jun 14, 2018

petebacondarwin requested a review from gkalpak June 15, 2018 07:29

petebacondarwin added this to REVIEW in docs-infra Jun 15, 2018

gkalpak approved these changes Jun 15, 2018

View reviewed changes

gkalpak 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 action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 15, 2018

petebacondarwin force-pushed the aio-hide-injectable-constructors branch from aca3297 to 2a76143 Compare June 15, 2018 12:08

petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jun 15, 2018

petebacondarwin modified the milestones: needsTriage, Backlog Jun 15, 2018

ngbot bot modified the milestones: Backlog, needsTriage Jun 15, 2018

gkalpak requested changes Jun 15, 2018

View reviewed changes

gkalpak added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jun 15, 2018

gkalpak removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Jun 15, 2018

fixup! build(docs-infra): support hiding constructors of injectable c…

a4fed6c

…lasses

petebacondarwin 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 Jun 22, 2018

petebacondarwin moved this from REVIEW to MERGE in docs-infra Jun 22, 2018

fixup! build(docs-infra): support hiding constructors of injectable c…

4feac28

…lasses

gkalpak approved these changes Jun 26, 2018

View reviewed changes

petebacondarwin added the action: merge The PR is ready for merge by the caretaker label Jun 26, 2018

jasonaden closed this in cf0968f Jun 26, 2018

gkalpak removed this from MERGE in docs-infra Jul 17, 2018

petebacondarwin deleted the aio-hide-injectable-constructors branch July 18, 2018 07:20

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build(docs-infra): support hiding constructors of injectable classes #24529

build(docs-infra): support hiding constructors of injectable classes #24529

petebacondarwin commented Jun 14, 2018 •

edited

mary-poppins commented Jun 14, 2018

gkalpak left a comment

gkalpak commented Jun 15, 2018

petebacondarwin commented Jun 15, 2018

gkalpak commented Jun 15, 2018

petebacondarwin commented Jun 15, 2018

petebacondarwin commented Jun 15, 2018

gkalpak Jun 15, 2018

gkalpak Jun 15, 2018

gkalpak Jun 15, 2018

gkalpak Jun 15, 2018

petebacondarwin Jun 15, 2018

jbogarthyde commented Jun 21, 2018

petebacondarwin commented Jun 22, 2018

mary-poppins commented Jun 22, 2018

jbogarthyde commented Jun 25, 2018

mary-poppins commented Jun 26, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

build(docs-infra): support hiding constructors of injectable classes #24529

build(docs-infra): support hiding constructors of injectable classes #24529

Conversation

petebacondarwin commented Jun 14, 2018 • edited

mary-poppins commented Jun 14, 2018

gkalpak left a comment

Choose a reason for hiding this comment

gkalpak commented Jun 15, 2018

petebacondarwin commented Jun 15, 2018

gkalpak commented Jun 15, 2018

petebacondarwin commented Jun 15, 2018

petebacondarwin commented Jun 15, 2018

gkalpak Jun 15, 2018

Choose a reason for hiding this comment

gkalpak Jun 15, 2018

Choose a reason for hiding this comment

gkalpak Jun 15, 2018

Choose a reason for hiding this comment

gkalpak Jun 15, 2018

Choose a reason for hiding this comment

petebacondarwin Jun 15, 2018

Choose a reason for hiding this comment

jbogarthyde commented Jun 21, 2018

petebacondarwin commented Jun 22, 2018

mary-poppins commented Jun 22, 2018

jbogarthyde commented Jun 25, 2018

mary-poppins commented Jun 26, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

petebacondarwin commented Jun 14, 2018 •

edited