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

Flag @advanced instead of @internal #2264

Open
jensscherbl opened this issue Mar 17, 2024 · 13 comments
Open

Flag @advanced instead of @internal #2264

jensscherbl opened this issue Mar 17, 2024 · 13 comments

Comments

@jensscherbl
Copy link

Just stumbled over the example for the system.loadPlugins:after hook in the guide and wanted to dynamically register an extensions based on a config option in a plugin like suggested in the documentation.

// register additional extensions like you would
// directly in the Kirby::plugin() call
kirby()->extend([
   // ...
], kirby()->plugin(...));

However, the \Kirby\Cms\AppPlugins::extend method mentioned in the example is marked as @internal.

/**
* Register all given extensions
*
* @internal
* @param \Kirby\Cms\Plugin $plugin|null The plugin which defined those extensions
*/
public function extend(...): array {...}

return $this->extensions;
}

Now I’m wondering if the guide or the DocBlock annotation is wrong here.

@texnixe
Copy link
Member

texnixe commented Mar 17, 2024

Unfortunately, it is not clear from the commit or PR that dates back to 2019 why many methods were marked as internal. The methods themselves are missing from the documentation, but nevertheless used in several contexts.

@bastianallgeier @distantnative Could you explain why this was done?

@jensscherbl
Copy link
Author

jensscherbl commented Mar 17, 2024

Thanks. I did some digging myself as well and read that @internal annotations were introduced to filter certain methods from the auto-generated reference docs at some point.

Another thought after reporting this was “since the hook callbacks are bound to the Kirby object, I could also use $this->extend(...) instead and tell myself that it qualifies as “internal use”, but it still leaves me with a feeling of uncertainty 😅

From my understanding, if a method is kinda excluded from the public API with this annotation, I shouldn’t rely on it for custom development.

@texnixe
Copy link
Member

texnixe commented Mar 17, 2024

I did some digging myself as well and read that @internal annotations were introduced to filter certain methods from the auto-generated reference docs at some point.

Exactly, that's what I meant. We filter them out of the docs, only to use them in several examples anyway, which is not really stringent.

@bastianallgeier
Copy link
Member

I think this is caused by losing track.

We add @internal for two reasons:

  1. we don't want it to appear in the reference

  2. we are not entirely sure about the implementation yet and we might want to change it later.

  3. is often related to 2. but not always.

There are methods that are simply too much for the object reference. They go too deep and would only be valuable on a very high level and with a lot of additional documentation and explanations.

I think that extend is totally fine like that. I don't see us deprecating or removing this any time soon. But it's also not a method that you would normally need to use. I don't remember how it made it into the hook docs tbh.

@texnixe
Copy link
Member

texnixe commented Mar 18, 2024

@bastianallgeier However, also methods like $site->visit() are part of the internal methods, although this is needed for multi-language routes and very weird that it's missing from the docs. I think sometimes this classification does not really make sense or seems at least arbitrary.

@bastianallgeier
Copy link
Member

@texnixe I think it's sometimes just massively outdated.

@distantnative
Copy link
Member

I wouldn't know how we could do this not in any arbitrary way. We have the competing goals of offering our user the best and most detailed information vs. not overwhelming our users (especially new ones) with a myriad of very technical methods - in the end they don't find the important basic ones amidst all those very advanced ones. I think any decision to draw a line - this is in, this is out - will be arbitrary. And will be wrong at times (or actually right, even if one user among hundreds was looking for it) with the need to revise them when we notice that our classification doesn't fit (anymore). I can't see how there could be an exact system of rules to decide.

@lukasbestle
Copy link
Member

Maybe it could help to differentiate:

  • @internal whenever we don't plan to keep compatibility and might change the behavior later ➡️ Hidden both in the reference and by IDEs
  • @advanced for methods that are pretty much stable but too obscure for most users ➡️ Accepted by IDEs as @advanced is non-standard, but we can hide those in the reference or keep them behind an advanced toggle

@bastianallgeier
Copy link
Member

@lukasbestle I like that suggestion a lot.

@jensscherbl
Copy link
Author

@lukasbestle Yes please, this would clarify a lot when digging deeper 😅

@texnixe
Copy link
Member

texnixe commented Mar 23, 2024

Guess this needs an issue in the kirby repo to change the doc blocks accordingly, then as a next step change our logic to show advanced methods.

@lukasbestle
Copy link
Member

What do you think @distantnative? If you agree on the concept, could you work on the implementation as well? I think you have the best overview of our docs reflection logic.

@distantnative
Copy link
Member

I'm fine with switching to @internal and @advanced. 👍

@distantnative distantnative changed the title Docs suggest usage of internal methods Flag @advanced instead of @internal May 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants