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

Doc comments #63

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

Doc comments #63

wants to merge 2 commits into from

Conversation

john-h-kastner-aws
Copy link
Contributor

Signed-off-by: John Kastner <jkastner@amazon.com>
@max2me
Copy link

max2me commented Apr 29, 2024

LGTM. I'm especially excited about support for doc comments in JSON policy format.

Copy link
Contributor

@cdisselkoen cdisselkoen left a comment

Choose a reason for hiding this comment

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

LGTM, with the caveat below about considering all of our syntax options

);
```

Doc comments may also be placed immediately prior schema elements to document the purpose of entity types and actions and before each attribute for an entity type or action context.
Copy link
Contributor

Choose a reason for hiding this comment

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

what about before principal and resource in an appliesTo block?

(I do acknowledge the comment in Drawbacks that we need not block this RFC on discussing additional positions for doc comments, and additional ones can be added later as a non-breaking change. We can punt on my comment here if we want.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These would also be good additions. I wasn't sure where to draw the line for what to include in this RFC. Hopefully extra comments positions can be added later without another RFC.

Comment on lines 110 to 113
### Alternative doc comment syntax

Choosing to use Rust style doc comments feels natural after developing Cedar in Rust, but we should consider alternative syntax options.
Some options include using block style doc comments (`/** .. */`), using another common comment character for doc comments (e.g., `#`), and picking a different third character following hte usual two slashes (e.g., `//@` to emphasize the connection with annotations).
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that we should consider all of these options; I'd like to hear others' thoughts on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@philhassey suggests just using // as in Godoc which I hadn't considered as an option. Not a huge fan, but it's worth considering.

Copy link
Contributor

Choose a reason for hiding this comment

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

No strong feelings on my end. I like the look of ///, but likely I'm biased from having mostly used Rust lately. My second preference would be //@ (or some other third character like ! or #).

```cedarschema
/// This entity defines our central user type
entity User {
/// The location where this user works. E.g., ABC17 or XYZ77.

Choose a reason for hiding this comment

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

In go doc comments are just comments // that are before an identifier. So fairly similar to the /// approach. Here's a link to the doc to read about it.

https://go.dev/blog/godoc

Here's an excerpt:

The convention is simple: to document a type, variable, constant, function, or even a package, write a regular comment directly preceding its declaration, with no intervening blank line. Godoc will then present that comment as text alongside the item it documents.

// Fprint formats using the default formats for its operands and writes to w.
// Spaces are added between operands when neither is a string.
// It returns the number of bytes written and any write error encountered.
func Fprint(w io.Writer, a ...interface{}) (n int, err error) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not a fan of using // for doc comments and regular comments. Feels like it could make parsing a pain, and it would be easy to misplace a doc comment without an way for the parser to notice and warn you.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also prefer a different syntax for doc comments vs. normal comments. Otherwise the two will only be distinguished by location in the policy/schema, which might make it difficult to figure out why a comment is (or isn't) rendering in the auto-generated documentation.

Choose a reason for hiding this comment

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

I saw it referenced but can you add the link to Rust Doc comments in the RFC. C# also uses /// for Documentation comments

@khieta khieta added the pending This RFC is pending; for definitions see the README label May 14, 2024
Copy link
Contributor

@khieta khieta left a comment

Choose a reason for hiding this comment

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

I'm happy with the proposal as-is (although I left some small comments).

```cedarschema
/// This entity defines our central user type
entity User {
/// The location where this user works. E.g., ABC17 or XYZ77.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also prefer a different syntax for doc comments vs. normal comments. Otherwise the two will only be distinguished by location in the policy/schema, which might make it difficult to figure out why a comment is (or isn't) rendering in the auto-generated documentation.

text/0063-doc-comments.md Outdated Show resolved Hide resolved
text/0063-doc-comments.md Outdated Show resolved Hide resolved
text/0063-doc-comments.md Outdated Show resolved Hide resolved
text/0063-doc-comments.md Outdated Show resolved Hide resolved
text/0063-doc-comments.md Outdated Show resolved Hide resolved
text/0063-doc-comments.md Outdated Show resolved Hide resolved
text/0063-doc-comments.md Outdated Show resolved Hide resolved
text/0063-doc-comments.md Outdated Show resolved Hide resolved

Comments do not exist in the JSON format, so this alternative provides nothing for documenting a JSON formatted policy or schema.

### Annotations as documentation
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an interesting option to me, but I still prefer the look of /// (or whatever syntax is chosen). I think it's fine to allow some overlap in functionality between annotations and doc comments. It might also be useful to unify their implementation in the future so that /// is syntactic sugar for @doc(...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The doc comment syntax being syntactic sugar under the hood is something I had in mind while writing this. It requires a substantial extension to extensions beyond ever RFC #48, but perhaps people will find other application of generalized annotations.

Co-authored-by: Kesha Hietala <khieta@amazon.com>
@sarahcec sarahcec added the archived This RFC is pending, but no action is currently planned -- please comment if you are interested label Jun 25, 2024
@john-h-kastner-aws
Copy link
Contributor Author

We've tagged this as archived because we think that doc-comments should sugar for @doc annotations regardless of what the doc-comment syntax is. We plan to first extend where annotations can be placed (do be determined based on the outcome of rfc #48). After that documentation tooling can be developed against the annotation API. We can then re-examine this RFC to introduce syntactic sugar for doc comments.

@john-h-kastner-aws john-h-kastner-aws removed the pending This RFC is pending; for definitions see the README label Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
archived This RFC is pending, but no action is currently planned -- please comment if you are interested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants