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 js_re.ml #5443

Merged

Conversation

nkrkv
Copy link
Contributor

@nkrkv nkrkv commented Jun 16, 2022

The case where MDX is arguably inferior to the comments. Please, review the content removed:

  • Intro
  • Some MDN links

@nkrkv nkrkv mentioned this pull request Jun 16, 2022
35 tasks
Copy link
Member

@ryyppy ryyppy left a comment

Choose a reason for hiding this comment

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

If your major concern is a less detailed intro, then it's fine to keep the stuff you deem important. We could also extend the manual with more useful Regex infos... wdyt?

Provides bindings for JavaScript Regular Expressions

{4 Syntax sugar}
ReScript provides a bit of syntax sugar for regex literals: `[%re "/foo/g"]`
Copy link
Member

Choose a reason for hiding this comment

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

I guess this part was removed since there is already some coverage in the manual: https://rescript-lang.org/docs/manual/latest/primitive-types#regular-expression

I guess it would generally be better to document the details in the manual instead to make the information more visible?

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 see your point. Agree, the intro is better suited for the manual.

So, I’d just live here the note telling the API is not immutable.

@nkrkv nkrkv force-pushed the sync-belt-doc-headers-js-re branch from f7dbc97 to 634b6b9 Compare June 18, 2022 20:39
@nkrkv
Copy link
Contributor Author

nkrkv commented Jun 18, 2022

Yep. I’ve recovered a few links to MDN. Now I consider the PR is ready for merge.

@ryyppy ryyppy merged commit b75250e into rescript-lang:sync-belt-doc-headers Jun 19, 2022
@ryyppy
Copy link
Member

ryyppy commented Jun 19, 2022

looking good. thanks!

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