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

RFC 0011: Extending API Docs #11

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

nobodywasishere
Copy link

This RFC is a follow up to discussion on the Crystal forum.

@nobodywasishere nobodywasishere changed the title RFC #11: Extending API Docs RFC 0011: Extending API Docs Nov 14, 2024
@crysbot
Copy link

crysbot commented Nov 15, 2024

This pull request has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/rfc-extending-api-docs-and-compiler-semantic-output/7314/22

@jgaskins
Copy link

Strongly in favor of this. I agree with some of the commenters on the forum that documenting methods that nobody but the library authors will ever use is not a great idea, but sometimes the intended audience of a protected method isn't you, but it also isn't everyone. People need to be able to see what methods are available to them in the context they're developing in.

In my case, I have an ORM called Interro where your query objects inherit from a framework type. Methods like where and order_by are protected, but that's not because Interro uses them internally — it doesn't use them at all. Interro implements them so that your query objects can use them, but they're protected so that the rest of your application can't be coupled to your DB schema. Here is a quick example:

struct PostQuery < Interro::QueryBuilder(Post)
  def by(author : User)
    where author_id: author.id
  end

  def published
    where { |post| post.published_at < Time.utc }
  end

  def in_reverse_chronological_order
    order_by created_at: :desc
  end
end

In this example, note that I'm implementing my query object's methods in terms of where and order_by. The way I use the PostQuery is to use these methods:

author_feed = PostQuery.new
  .by(author)
  .published
  .in_reverse_chronological_order

If I were able to call where or order_by from outside of PostQuery (for example, in the endpoint that renders the feed), my application would become coupled to the specifics of my DB schema. Since they're protected, I maintain a layer of separation so that if I change the mechanics of what it means for a post to be published or if I add the ability for multiple users to be listed as authors of a post, I can isolate those changes to these methods and the rest of my application does not need to change.

Application developers should be able to see what methods are available to them inside their query objects, so it's important to be able to document them at the very least.

Copy link
Collaborator

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

Thank you for this proposal 🙇

text/0011-extending-api-docs.md Outdated Show resolved Hide resolved
text/0011-extending-api-docs.md Show resolved Hide resolved
text/0011-extending-api-docs.md Outdated Show resolved Hide resolved
text/0011-extending-api-docs.md Show resolved Hide resolved
text/0011-extending-api-docs.md Outdated Show resolved Hide resolved
text/0011-extending-api-docs.md Outdated Show resolved Hide resolved
text/0011-extending-api-docs.md Outdated Show resolved Hide resolved
text/0011-extending-api-docs.md Outdated Show resolved Hide resolved
text/0011-extending-api-docs.md Outdated Show resolved Hide resolved
text/0011-extending-api-docs.md Outdated Show resolved Hide resolved
text/0011-extending-api-docs.md Outdated Show resolved Hide resolved
text/0011-extending-api-docs.md Outdated Show resolved Hide resolved
text/0011-extending-api-docs.md Outdated Show resolved Hide resolved
@RX14
Copy link

RX14 commented Nov 16, 2024

Definitely in favour of this proposal, it seems useful, and the design is in keeping with the rest of the doc generator. One thing to keep an eye on is that the doc generator makes the visibility (protected/private) of the method clear and obvious in the generated docs. It should be impossible to miss.

@sol-vin
Copy link

sol-vin commented Nov 16, 2024

If I can suggest one possibility, when a lib fun gets documented can it include the name of the c function it's binding in the docs as well.

nobodywasishere and others added 3 commits November 16, 2024 09:08
Co-authored-by: Stephanie Wilde-Hobbs <[email protected]>
Co-authored-by: Stephanie Wilde-Hobbs <[email protected]>
Co-authored-by: Stephanie Wilde-Hobbs <[email protected]>
@nobodywasishere
Copy link
Author

Definitely in favour of this proposal, it seems useful, and the design is in keeping with the rest of the doc generator. One thing to keep an eye on is that the doc generator makes the visibility (protected/private) of the method clear and obvious in the generated docs. It should be impossible to miss.

Other people in the past have mentioned having separate sections for private and protected methods, would that work? My previous implementation just put the visibility next to the method name (as it would look in code)

@RX14
Copy link

RX14 commented Nov 17, 2024

Other people in the past have mentioned having separate sections for private and protected methods, would that work? My previous implementation just put the visibility next to the method name (as it would look in code)

I'm ambivalent about having a separate section, I mostly wanted to make a point about the visual distinctiveness of the visibility modifier when looking at a single method. You can't rely on sections for this because the section is context you can forget if the section marker is off-screen, and therefore doesn't contribute to being immediately obvious.

@nobodywasishere nobodywasishere marked this pull request as ready for review December 3, 2024 03:49
@nobodywasishere
Copy link
Author

Is there anything else to do for this RFC to be merged? Want to get started on this for Crystal 1.15 if possible.

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

LGTM.
I only have some minor suggestions to polish the language. But none of them are blockers.

The general is well received, so I think we can already start on the implementation and finish the RFC in parallel.

I expect that we might only notice some issues that need clarification by using an actual implementation and testing it.

text/0011-extending-api-docs.md Outdated Show resolved Hide resolved

# Motivation

Currently, API documentation is not generated for private/protected methods/objects or C lib binding objects.
Copy link
Member

Choose a reason for hiding this comment

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

polish: Perhaps it would be helpful to use a single term for documented things (methods, types, constants, macros, functions, ...). I think "object" fits great. Maybe we could introduce that here and mention what it refers to, and then later occurences in the text could simplify to the term "(documented) objects"? (This is already happening in some places, actually)

For example:

@@ -23,1 +23,1 @@
-The `:showdoc:` directive can be added to private or protected objects, methods, and C lib bindings, to have them show up in API documentation.
+The `:showdoc:` directive can be added to `private` or `protected` objects, and objects in `lib` bindings, to have them show up in API documentation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about the "object" word. It's very connoted for me as type instances and has zero relationship with methods or C symbols 😕

Copy link
Member

Choose a reason for hiding this comment

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

Maybe "features" then?

text/0011-extending-api-docs.md Outdated Show resolved Hide resolved
nobodywasishere and others added 2 commits December 11, 2024 18:09
Co-authored-by: Johannes Müller <[email protected]>
Co-authored-by: Johannes Müller <[email protected]>
@crysbot
Copy link

crysbot commented Jan 21, 2025

This pull request has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/win32cr-1-0-release/7630/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants