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

Re-export traits at top level to show them in top level doc page #73

Closed
twitu opened this issue Oct 8, 2022 · 6 comments
Closed

Re-export traits at top level to show them in top level doc page #73

twitu opened this issue Oct 8, 2022 · 6 comments
Assignees
Labels
documentation Improvements or additions to documentation enhancement New feature or request

Comments

@twitu
Copy link
Collaborator

twitu commented Oct 8, 2022

The top level doc page for the package shows the modules but not the traits it offers.

Since the primary aim of this repo is to capture common patterns as traits. They can be exposed at the top level. This is will also expose those traits in the top level documentation. This can be done by re-exporting them from lib.rs.

Again we can take a cue from serde docs and lib.rs

@encody encody self-assigned this Oct 10, 2022
@encody encody added documentation Improvements or additions to documentation enhancement New feature or request labels Oct 10, 2022
@encody
Copy link
Member

encody commented Oct 10, 2022

With the resolution of this issue comes the possibility for future item name collisions. For example, the approval module has a trait called Action, which is a name I could see being easily used elsewhere. Exporting this name at the crate level could then lead to confusion / collision / breaking changes if we ever introduce a component that also uses an item called Action. However, it seems that the Rust community generally prefers to use the path as a disambiguating prefix.

So, it should be decided:

  1. Whether to fix this issue as recommended above.
  2. If we decide to fix this issue as recommended, either:
    1. We only export a limited selection of the traits. Which ones?
    2. We prefix the item names with the module names. I don't like this because it is superfluous (that information already exists in the form of the canonical module path, e.g. nep141::Nep141Hook as opposed to nep141::Hook) and it goes against Rust convention.

However, this also exposes an internal naming inconsistency: Things like Nep141Hook, etc. would be renamed to merely Hook. I'm personally okay with this change (breaking as it may be) because of the reasons given in 2.ii above.

@ryancwalsh As arbiter of the properly verbose, do you have input on this topic?

@ryancwalsh
Copy link
Contributor

Thanks for raising this, @twitu . And thanks for the options, @encody .

Remember, I have nearly zero Rust experience, so it would probably be best to seek others' opinions.

From my instinct, my answer to # 1 is leaning towards Yes.

But I've read 2.ii. and the "However" sentence many times and am having trouble following along. What is "However" responding to? And what does the "this" in that sentence refer to?

I don't know that I really understand the problem of naming things like nep141::Hook (which to me seems a bit better than nep141::Nep141Hook which does look redundant). If there is another Hook in the same crate, does that really cause a "collision"?

Wouldn't it just mean that people who are importing those functions would need to specify the longer (clearer) import name to disambiguate?

What would be an example of how that would work? What are the downsides?

@twitu
Copy link
Collaborator Author

twitu commented Oct 10, 2022

While working on #74, I've observed a couple of other things that are relevant here.

This crate offers two main items traits and macros for various patterns. While the traits are useful, it will be uncommon for someone to derive a custom implementation. It's the macros that are really important and will be used the most. They give default implementations and is likely to be the standard. Considering this, it's the macros that should be highlighted in the top level docs because it's most likely that the someone looking at the docs will want to read about the macros.

My second observation is that most of the components are actually made of two traits one for internal methods and one for external functions for e.g. Owner and OwnerExternal. Both together make the owner component useful, and the even derive macro for Owner derives them both.

Considering this, exporting traits at the top level may not be the best idea because both the traits will have to be exported. So exporting the component module at the top level (for e.g. owner), along with descriptive module level docs (in progress in #74) seems to be a good fit here. This is currently the state of the doc so no changes are needed for this.

An immediate improvement to consider is to hyperlink items everywhere in the docs and especially hyperlink the traits in the single line description shown for the derive macros here.

@ryancwalsh
Copy link
Contributor

@twitu Great!

@encody
Copy link
Member

encody commented Oct 11, 2022

Sounds like we are not implementing the titular suggestion, but instead 1) a uniform naming convention, and 2) a documentation improvement. Is this correct? If so, this issue should probably be closed and two new ones created.

@twitu
Copy link
Collaborator Author

twitu commented Oct 13, 2022

Consistent naming is being discussed in #70

@twitu twitu closed this as completed Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants