typedoc icon indicating copy to clipboard operation
typedoc copied to clipboard

Add option hiddenCategories

Open tobiasschweizer opened this issue 3 years ago • 14 comments

resolves #1407

This PR is not ready for review yet.

tobiasschweizer avatar Dec 07 '20 09:12 tobiasschweizer

@Gerrit0 I could already add the declarations as described in https://github.com/TypeStrong/typedoc/issues/1407#issuecomment-739329923

However, as for the category check I struggle a bit.

I would expect some logic as follows:

  • get the category of the class that is currently being processed
  • if the current class's category is contained in hiddenCategories, use project.removeReflection

tobiasschweizer avatar Dec 07 '20 09:12 tobiasschweizer

If I understand correctly, there are three processing steps defined for the category plugin:

  1. onBegin
  2. onResolve
  3. onEndResolve

So would this have to go into onEndResolve (when everything is done) like:

    private onEndResolve(context: Context) {
        const project = context.project;
        this.categorize(project);
        if (this.hiddenCategories) {
             const reflections: Reflection[] = [];
             // check if `this.hiddenCategories` contains categories from `project`
             // push those reflections on `reflections `
             ...
             
             // remove reflections belonging to hidden categories from the documentation
             reflections.forEach(reflection => project.removeReflection(reflection))
        }
    }

How could I write a test for the new option?

tobiasschweizer avatar Dec 07 '20 17:12 tobiasschweizer

You have the right idea - however, unfortunately due to the architecture of some of the other plugins (GroupPlugin is one of them), removing reflections after the onBegin method will break things... which I completely forgot about when I said this should be an easy issue. I haven't build a solution for this problem yet - to handle it properly the removeReflection method would need to emit an event so plugins that store reflection data can properly remove it... I think there is an issue open for this somewhere. This has been on the list for a while, I just haven't gotten around to it since I haven't needed it, and doing it properly requires reviewing all the existing plugins to ensure they handle the event properly... I'm looking into how difficult this is now, I might be able to finally do it this week.

Adding a test for this is pretty easy at least, I think this option can always be set, so we don't need a new spec file type:

  • In scripts/rebuild_specs.js and src/test/converter.test.ts add a hidden category in the options passed to app.bootstrap
  • In src/test/converter/comment add a new file exporting some values - some with the category tag, some without.
  • Run npm run rebuild_specs converter comment to generate a specs.json file with your changes

Gerrit0 avatar Dec 09 '20 03:12 Gerrit0

@Gerrit0 I understand :-) So ping me once you have implemented the necessary functionality for removeReflection and then I will try to finish this PR.

tobiasschweizer avatar Dec 09 '20 08:12 tobiasschweizer

@Gerrit0 just a shot in the dark: What about trying not to add the reflections that should be hidden/excluded? So we would not have to remove them.

tobiasschweizer avatar Dec 11 '20 17:12 tobiasschweizer

That could work as well - however that means that the categorization logic would have to happen earlier - instead of in the resolve event.... I'm not completely against that.

Gerrit0 avatar Dec 13 '20 19:12 Gerrit0

I spent a few hours today trying to get the reflection removed event to work - this is trickier than I anticipated... TypeDoc is really bad about storing data in multiple places, and it is really easy to miss one of them... I think I mostly have it working except for inheritance. (@hidden comment on Parent in class Child extends Parent should totally hide parent... but it is only half being hidden)

Gerrit0 avatar Dec 14 '20 03:12 Gerrit0

@Gerrit0 sorry that I cannot help you there as I am not familiar with the architecture. Which branch are you working on?

tobiasschweizer avatar Dec 14 '20 08:12 tobiasschweizer

I never ended up pushing any of the changes for the removal event. There are more architectural difficulties than I realized.

  1. If removal can happen after the resolve step, every plugin which sets properties on reflections that reference another reflection needs to keep track of what properties are set so if that referenced reflection is removed, the reference can be removed.
  2. Allowing removal during the resolve step is especially problematic since the order that reflections are resolved in can affect the resulting output.

I think I'll need to dedicate a couple solid days to really going through and understanding all of the relations in order to properly handle this. I think the "right" solution is to do removal after resolution, but I also really dislike that solution since it adds a ridiculous amount of property tracking... which would be really easy to mess up. This probably won't happen this year, so unfortunately I think the best bet for this feature is to do filtering in onBeginResolve like the comment plugin does.

#1177 has some more notes on what "removal" ought to mean.

Gerrit0 avatar Dec 20 '20 19:12 Gerrit0

@lastthyme @Gerrit0 sorry, but this PR is not yet ready for review.

tobiasschweizer avatar May 03 '21 06:05 tobiasschweizer

@Gerrit0 Hi, any chance to revive this PR? ;-)

tobiasschweizer avatar May 30 '22 06:05 tobiasschweizer

This probably won't happen this year, so unfortunately I think the best bet for this feature is to do filtering in onBeginResolve like the comment plugin does. - Me 2 years ago

This probably won't happen ever. It massively complicates a ton of logic. If updated to do as described, I'm still open to this.

Gerrit0 avatar May 30 '22 14:05 Gerrit0

This probably won't happen this year, so unfortunately I think the best bet for this feature is to do filtering in onBeginResolve like the comment plugin does. - Me 2 years ago

This probably won't happen ever. It massively complicates a ton of logic. If updated to do as described, I'm still open to this.

Shall I close this PR since I won't be able to work on this?

tobiasschweizer avatar May 31 '22 08:05 tobiasschweizer

It's still possible to do this - it just needs to be done at an earlier state. Removal needs to happen at EVENT_BEGIN_RESOLVE instead of later

Gerrit0 avatar May 31 '22 12:05 Gerrit0

cool :-)

tobiasschweizer avatar Sep 04 '23 19:09 tobiasschweizer