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

Document the backend #2365

Merged
merged 4 commits into from
Nov 25, 2020
Merged

Document the backend #2365

merged 4 commits into from
Nov 25, 2020

Conversation

CraftSpider
Copy link
Contributor

@CraftSpider CraftSpider commented Nov 20, 2020

Makes the backend crate #![deny(missing_docs)], and documents all public values. Motivation is making contribution and development a little easier.

Copy link
Contributor

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Always nice seeing things documented!

I'm a bit worried though in that most of the documentation here just sort of looks at the struct/field name and doesn't document much more than the meaning behind that. I think it would be great to document with background knowledge how these fields interact, but if it's just reiterating the english name of the struct and the field then I'm worried that this is just checking off a box of "everything documented" without actually being too useful.

Type(ImportType),
/// Importing a JS enum
Copy link
Contributor

Choose a reason for hiding this comment

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

I've personally never been a huge fan of docs like on this enum, the documentation doesn't really expand any more than the english name of the type itself, e.g. that ImportKind::Function is "importing a function" I think is pretty obvious. I feel that docs like this tend to clutter things up without adding much value unfortuantely.

pub structural: bool,
/// For testing purposes
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be expanded with for what testing purposes this is used for?

(this is a public-facing attribute IIRC so it's not entirely just testing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Practically, it causes the Builder for the JS to bail if it finds itself generating code for something with assert_no_shim. I'll add an explanation of the behavior.

pub kind: ImportFunctionKind,
/// The shim name to use in the generated code
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW the "shim" here I think could use more explanation. It's not 100% clear from nothing what the shim even is here I think.

@CraftSpider
Copy link
Contributor Author

I at least tried to make sure to look through the 'usage' of each field, to make sure I didn't say anything false. But I agree some of them aren't much more useful than the name, and will admit to being motivated partially by a desire for completionism. Overall, at least with the struct names, I think it straddles the line but leans towards useful. EG with ImportFunction, 'A function being imported from JS' is mostly similar, but provides at least one useful confirmation: That 'import' means 'from JS'.
I'll make a second pass through and see if there are any subtleties I can mention (EG, on the ImportKind it's probably worth it to mention how it looks for that object / what makes it different from the others), and generally try to make sure they are useful for development. If you would like, I can strip out comments that don't end up providing more than a repetition of the name, and remove the deny. I personally prefer the style, but not so much that I would block the rest on it :P

@alexcrichton
Copy link
Contributor

Ok well in any case the words aren't hurting anything! I'll go ahead and merge, and thanks for the PR!

@alexcrichton alexcrichton merged commit 83cc988 into rustwasm:master Nov 25, 2020
@CraftSpider
Copy link
Contributor Author

Thank you. I'll try to make a follow-up with any improvements, once I have the time (With the holiday, we'll see whether it happens this week or not).

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

Successfully merging this pull request may close these issues.

2 participants