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

Add Syntax page #200

Merged
merged 5 commits into from
Oct 31, 2024
Merged

Add Syntax page #200

merged 5 commits into from
Oct 31, 2024

Conversation

feihong
Copy link
Collaborator

@feihong feihong commented Oct 18, 2024

Addresses #38

@@ -103,6 +103,7 @@ export default defineConfig({
items: [
{ text: "What is Melange", link: "/what-is-melange" },
{ text: "Rationale", link: "/rationale" },
{ text: "Syntaxes", link: "/syntaxes" },
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's in its own page, but maybe it should be merged into Getting Started instead?

Copy link
Member

Choose a reason for hiding this comment

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

For some reason "Syntaxes" doesn't sound good to me (?), maybe it's just an impression but would "Supported syntaxes" make more sense or I'm tripping?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed to "Supported syntaxes" in 5bacbf3

docs/syntaxes.md Outdated
Comment on lines 24 to 26
"opam-check-npm-deps" {with-test}
"ocaml-lsp-server" {with-test}
"dot-merlin-reader" {with-test}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should with-test be replaced by dev? I copied this directly from melange-opam-template.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'd replace it as 2.2 is released now. It is with-dev-setup though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jchavarri I created PR to update melange-opam-template: melange-re/melange-opam-template#38

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to with-dev-setup in 5bacbf3

docs/syntaxes.md Outdated Show resolved Hide resolved
@feihong feihong marked this pull request as ready for review October 18, 2024 02:09
@feihong
Copy link
Collaborator Author

feihong commented Oct 23, 2024

cc @psb

Copy link
Member

@jchavarri jchavarri left a comment

Choose a reason for hiding this comment

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

Looking good, it'd be great if @davesnx could also check as he suggested the idea of new "Syntaxes" section.

cc @anmonteiro

docs/syntaxes.md Outdated Show resolved Hide resolved
docs/syntaxes.md Outdated
Comment on lines 24 to 26
"opam-check-npm-deps" {with-test}
"ocaml-lsp-server" {with-test}
"dot-merlin-reader" {with-test}
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'd replace it as 2.2 is released now. It is with-dev-setup though.

docs/syntaxes.md Outdated
opam install -y . --deps-only
```

Note that reason support is already set up for you in
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Note that reason support is already set up for you in
Note that Reason support is already set up for you in

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 5bacbf3

docs/syntaxes.md Outdated
Note that reason support is already set up for you in
[melange-opam-template](https://github.com/melange-re/melange-opam-template).

## Editor configuration
Copy link
Member

Choose a reason for hiding this comment

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

I would mention that this is the "Editor configuration for Reason syntax" and somehow reference the previously existing Editor conf section.

Alternatively, move this editor conf section to /getting-started.html to consolidate all editor config under a single section, and maybe specify the part that is Reason specific with a link to /syntaxes page.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added link to "Editor integration" section of getting-started. Also added a link from "Editor integration" back to this page. It seems a bit circular, but I figure that explaining how to set up format-on-save for Reason syntax doesn't necessarily make sense if it hasn't been explained what Reason syntax is.

ac4e7d6

docs/syntaxes.md Outdated Show resolved Hide resolved
Copy link
Member

@davesnx davesnx left a comment

Choose a reason for hiding this comment

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

I would add a section on "Formatters" explaining refmt for Reason and ocamlformat for OCaml

docs/syntaxes.md Outdated
Comment on lines 13 to 14
To install Reason syntax support add `reason` to the `depends` section of the
`<project-name>.opam` file in your project:
Copy link
Member

Choose a reason for hiding this comment

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

I would expect a link to reasonml docs somewhere here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added links to both OCaml syntax and Reason syntax in the opening paragraph: 5bacbf3

docs/syntaxes.md Outdated
@@ -0,0 +1,64 @@
# Syntaxes

Melange supports two syntaxes: OCaml syntax (the default) and Reason syntax.
Copy link
Member

Choose a reason for hiding this comment

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

I would add a paragraph on what we mean by syntaxes, before we say melange supports both

Copy link
Contributor

Choose a reason for hiding this comment

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

I would mention that the syntax for the melange website can be toggled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I explain how to toggle syntaxes in 5bacbf3

I would add a paragraph on what we mean by syntaxes, before we say melange supports both

I added an example snippet before explaining the difference in 5bacbf3. Is that good enough?

docs/syntaxes.md Outdated

## Editor configuration

To enable format-on-save in VS Code:
Copy link
Member

Choose a reason for hiding this comment

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

Probably link to reasonml editor for more info? and same for OCaml?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What page do you want to link to?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added link in c36a87e

Copy link
Collaborator Author

@feihong feihong left a comment

Choose a reason for hiding this comment

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

I would add a section on "Formatters" explaining refmt for Reason and ocamlformat for OCaml

Added a Formatters section in 5bacbf3

docs/syntaxes.md Outdated
Comment on lines 24 to 26
"opam-check-npm-deps" {with-test}
"ocaml-lsp-server" {with-test}
"dot-merlin-reader" {with-test}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jchavarri I created PR to update melange-opam-template: melange-re/melange-opam-template#38

@@ -103,6 +103,7 @@ export default defineConfig({
items: [
{ text: "What is Melange", link: "/what-is-melange" },
{ text: "Rationale", link: "/rationale" },
{ text: "Syntaxes", link: "/syntaxes" },
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed to "Supported syntaxes" in 5bacbf3

docs/syntaxes.md Outdated
@@ -0,0 +1,64 @@
# Syntaxes

Melange supports two syntaxes: OCaml syntax (the default) and Reason syntax.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I explain how to toggle syntaxes in 5bacbf3

I would add a paragraph on what we mean by syntaxes, before we say melange supports both

I added an example snippet before explaining the difference in 5bacbf3. Is that good enough?

docs/syntaxes.md Outdated
Comment on lines 13 to 14
To install Reason syntax support add `reason` to the `depends` section of the
`<project-name>.opam` file in your project:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added links to both OCaml syntax and Reason syntax in the opening paragraph: 5bacbf3

docs/syntaxes.md Outdated
Comment on lines 24 to 26
"opam-check-npm-deps" {with-test}
"ocaml-lsp-server" {with-test}
"dot-merlin-reader" {with-test}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to with-dev-setup in 5bacbf3

docs/syntaxes.md Outdated
opam install -y . --deps-only
```

Note that reason support is already set up for you in
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 5bacbf3

docs/syntaxes.md Outdated Show resolved Hide resolved
docs/syntaxes.md Outdated

## Editor configuration

To enable format-on-save in VS Code:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What page do you want to link to?

docs/syntaxes.md Outdated
Note that reason support is already set up for you in
[melange-opam-template](https://github.com/melange-re/melange-opam-template).

## Editor configuration
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added link to "Editor integration" section of getting-started. Also added a link from "Editor integration" back to this page. It seems a bit circular, but I figure that explaining how to set up format-on-save for Reason syntax doesn't necessarily make sense if it hasn't been explained what Reason syntax is.

ac4e7d6

@feihong feihong force-pushed the syntax-page branch 2 times, most recently from 7e12f37 to 030b370 Compare October 30, 2024 02:47
docs/syntaxes.md Outdated
the [OCaml Platform VS Code
extension](https://marketplace.visualstudio.com/items?itemName=ocamllabs.ocaml-platform),
works well with Reason, but some tools aren't completely aware of it—for
example, error messages from the compiler still use OCaml syntax.
Copy link
Member

@davesnx davesnx Oct 30, 2024

Choose a reason for hiding this comment

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

I would either link the issue here or not even mention this, since it's not always the case. Sometimes the error gets in reason and sometimes it doesn't

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Deleted that line in 55b49d4

Copy link
Member

@davesnx davesnx left a comment

Choose a reason for hiding this comment

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

Looks awesome, good job @feihong

Copy link
Member

@jchavarri jchavarri left a comment

Choose a reason for hiding this comment

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

Thanks!

On the command line, run

```bash
opam install -y . --deps-only
Copy link
Member

Choose a reason for hiding this comment

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

Should we mention that alternatively, users can just run opam install reason -y?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added alternate command in 55b49d4

docs/syntaxes.md Outdated Show resolved Hide resolved
@feihong feihong merged commit 0d54e6c into master Oct 31, 2024
@feihong feihong deleted the syntax-page branch October 31, 2024 02:02
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.

4 participants