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

Sync docs for belt_Set.mli #5428

Merged
merged 7 commits into from
Jun 13, 2022
Merged

Sync docs for belt_Set.mli #5428

merged 7 commits into from
Jun 13, 2022

Conversation

whitchapman
Copy link
Contributor

Please let me know if I am on the right track. I am basically replacing the comments with the raw comments from rescript-lang.

@whitchapman whitchapman mentioned this pull request Jun 11, 2022
35 tasks
@whitchapman
Copy link
Contributor Author

I was attempting to stick with doing all of the Set modules but ran into a couple of issues:

  • there is not a corresponding mdx file for belt_Set.cppo.mli [and I do not know what cppo indicates]
  • when looking at belt_MutableSet.mli, noticed inconsistency between the usage of ('k,'id) and ('value, 'id) -- I figure this should be consistent throughout the modules but that would require changing code. Please advise.

@ryyppy
Copy link
Member

ryyppy commented Jun 13, 2022

Sorry for my late review.

Great work so far! The most important thing is to remove the leading res sig for each doc string, since it would just duplicate the actual function signature.

@whitchapman
Copy link
Contributor Author

whitchapman commented Jun 13, 2022

...remove the leading res sig for each doc string, since it would just duplicate the actual function signature.

I removed all duplicate signatures from all 6 files. Seems obvious in hindsight 😄 @ryyppy

@ryyppy
Copy link
Member

ryyppy commented Jun 13, 2022

looking good now. thanks!

@ryyppy ryyppy merged commit 712a52f into rescript-lang:sync-belt-doc-headers Jun 13, 2022
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.

None yet

2 participants