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

Fix: Don't link to undocumented types in API docs #14878

Conversation

spuun
Copy link
Contributor

@spuun spuun commented Aug 7, 2024

Fixes #9816

@Blacksmoke16 Blacksmoke16 added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:tools:docs-generator labels Aug 7, 2024
@spuun spuun changed the title Don't reference nodoc types in generated html Don't link to undocumented types in api docs Aug 7, 2024
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
@spuun
Copy link
Contributor Author

spuun commented Aug 8, 2024

Maybe it would make more sense to pass type to these *_with_docs methods…

@spuun spuun marked this pull request as ready for review August 8, 2024 05:50
@straight-shoota
Copy link
Member

Maybe it would make more sense to pass type to these *_with_docs methods…

Why? What would that improve? If they're instance methods on TypeTemplate and type is an instance variable, why would we pass it as method parameter?

@spuun
Copy link
Contributor Author

spuun commented Aug 8, 2024

Maybe it would make more sense to pass type to these *_with_docs methods…

Why? What would that improve? If they're instance methods on TypeTemplate and type is an instance variable, why would we pass it as method parameter?

To make it clear in the html template from where the types are fetched, but maybe one should understand that from the context.

@straight-shoota
Copy link
Member

I see. Maybe it could make sense to move the *_with_docs methods to Type. But then we wouldn't have access to types.
So I think this is fine as it is. We can always refactor later.

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

I understand @Sija's feeling: I liked the type.subclasses notation that was a little more explicit (i.e. I want the subclasses of type). We can't do that because we need types from the template, and filtering in the template sounds like the correct place... so what about a filter method, for example with_docs(type.subclasses)?

Now this is nitpicking and the PR can be merged as-is 👍

@straight-shoota straight-shoota added this to the 1.14.0 milestone Aug 13, 2024
@straight-shoota straight-shoota changed the title Don't link to undocumented types in api docs Fix: Don't link to undocumented types in API docs Aug 14, 2024
@straight-shoota straight-shoota merged commit 78c9282 into crystal-lang:master Aug 14, 2024
62 checks passed
@dentarg dentarg mentioned this pull request Aug 14, 2024
@Blacksmoke16 Blacksmoke16 linked an issue Aug 14, 2024 that may be closed by this pull request
@Blacksmoke16
Copy link
Member

Blacksmoke16 commented Aug 14, 2024

I noticed while looking into the latest comment on #12018, that this PR may have changed more than we expected?

Before:
image

After:
image

The Crystal::System::* and IO::Evented make sense because those were :nodoc:, but the other like IO::FileDescriptor I would have expected to remain given they're public?

I can move this to its own issue if this is indeed a regression.

@straight-shoota
Copy link
Member

Yeah, that doesn't seem to work correctly. I believe types only contains the top-level types (see Generator#collect_subtypes).

@spuun
Copy link
Contributor Author

spuun commented Aug 15, 2024

I believe the filtering must be done in Doc::Type instead? I see it's done in Doc::Type#subclasses but not in #ancestors, #included_modules or #extended_modules. I'll give it another try including specs.

@Blacksmoke16 Blacksmoke16 removed a link to an issue Aug 15, 2024
@straight-shoota straight-shoota removed this from the 1.14.0 milestone Sep 4, 2024
@straight-shoota straight-shoota added the status:reverted PR was reverted or reverts another one, and is not part of a milestone label Sep 10, 2024
@straight-shoota
Copy link
Member

This patch was eventually reverted in #14908 due to #14878 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. status:reverted PR was reverted or reverts another one, and is not part of a milestone topic:tools:docs-generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Doc generator creates links to features in excluded namespaces
5 participants