Integrate open PRs and update instructions #239

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

Jump to bottom

Merged

aspeddro merged 14 commits into rescript-lang:main from Emilios1995:main

Mar 20, 2024

Merged

Integrate open PRs and update instructions #239

aspeddro merged 14 commits into rescript-lang:main from Emilios1995:main

Mar 20, 2024

Conversation

Emilios1995

Copy link

Contributor

@Emilios1995 Emilios1995 commented Mar 11, 2024 •

edited

Loading

Before the transfer, I made a fork and merged all the PRs that looked good to me. This set of PRs includes the one that refactors the repo so that it's easy to install everything without the separate nvim-treesitter-rescript. I also updated the Readme so that the install instructions point to the new location. (I also updated an outdated reference to the InspectTree command.)

The PRs are: #230, #232, #233, #235, #237

After this is merged into main, I'll try to get this parser included in Neovim by default to make it even easier to install.

Failing wild tests

These are the missing features that are causing the wild tests to fail:

Failing tests:

as annotations on record fields.

Source: /rescript-nodejs.git/src/WorkerThreads.res

Snippet:

type options<'a, 'env> = {
 argv: array<string>,
 env: {..} as 'env,
 // ...
 }

Optional fields in switch statements (it also fails in deconstruction patterns in let bindings)

Source: /rescript-lang.org/scripts/gendocs.res:152

Snippet:
```
 switch docItem {
 | Value({id, name, docstrings, signature, ?deprecated}) => {
 //...
```
Partial application

Sources:
- rescript-lang.org/src/Playground.res:1637
- rescript-lang.org/src/SyntaxLookup.res:121
- rescript-lang.org/src/bindings/RescriptCompilerApi.res:247
- rescript-lang.org/src/common/BlogFrontmatter.res:127
- rescript-association/rescript-lang.org/src/common/DocFrontmatter.res:11
- rescript-association/rescript-lang.org/src/vendor/Json_decode.res:198
Snippet:
```
Js.Array2.map(locMsgs, locMsgToCmError(~kind=#Error, ...))
```

cristianoc and others added 8 commits

October 11, 2023 16:53

@cristianoc


 Fix infinite loop in scanner.

c85307e

@zweimach @Emilios1995


 feat: dynamic module import

b83d7eb

@zweimach @Emilios1995


 feat: record type spread

b50f638

@zweimach @Emilios1995


 fix: negative decimal integer literal

3dfdf8a

@Emilios1995


 Fix injections

c454e09

@Emilios1995


 Support GraphQA injection for rescript-relay

5ca149c

@TheFedaikin @Emilios1995


 feat(plugin): installation

c379199

update queries structure, add native nvim-treesitter.parsers instructions

@Emilios1995


 Update instructions

c03220f

@Emilios1995 Emilios1995 mentioned this pull request

Mar 11, 2024

Future maintenance of this package #238

Closed

@Emilios1995

Copy link

Contributor Author

Emilios1995 commented Mar 12, 2024

Looks like the wild tests are failing. Those are useful, but I don't think it's a good idea to run them as checks on PRs since a change upstream could cause something to fail even if it's completely unrelated to the PR.

@aspeddro

Copy link

Collaborator

aspeddro commented Mar 12, 2024 •

edited

Loading

#233 should parse type paramaters in type spread

An error example from test wild: https://github.com/rescript-lang/tree-sitter-rescript/actions/runs/8234279999/job/22515674801?pr=239#step:8:26

cca-io/rescript-material-ui/packages/rescript-mui-material/src/components/FilledInput.res

type props<'value> = {
 ...InputBase.publicProps<'value>
}

@Emilios1995


 Fix generic type spread

309fd25

@aspeddro

Copy link

Collaborator

aspeddro commented Mar 17, 2024

Since we updated the installation instructions, we should remove src from .gitignore

@Emilios1995

Copy link

Contributor Author

Emilios1995 commented Mar 18, 2024

Since we updated the installation instructions, we should remove src from .gitignore

That's probably right. I think we could also ask users to run TSInstallFromGrammar, but checking the generated parser in is simpler for sure. But also, I think we should change the requires_generate_from_grammar option in the installation instruction snippet to false, right?

@Emilios1995

Copy link

Contributor Author

Emilios1995 commented Mar 18, 2024 •

edited

Loading

Since we updated the installation instructions, we should remove src from .gitignore

That's probably right. I think we could also ask users to run TSInstallFromGrammar, but checking the generated parser in is simpler for sure. But also, I think we should change the requires_generate_from_grammar option in the installation instruction snippet to false, right?

@aspeddro I looked it up. We shouldn't need to check in the files in src because the snippet in the instructions has requires_generate_from_grammar = true. When that option is set, Neovim will generate the parser regardless of whether the user called TSInstall or TSInstallFromGrammar. See here.

Of course, I'm open to checking in the generated files if you think that'll help with other editors, but this argument from the maintainer of the Swift parser for not checking it in makes sense to me.

Two more things:

You may have seen that I fixed the type spreads wild test that was failing (if not, see my previous commit.) However, when looking at the tree, I thought it would be much nicer to have separate nodes for type spreads. I made that change in a PR against my main, which is the target of this PR. Please take a look, and if you agree, I'll integrate it here.
I edited the description of this PR with the missing features that are making the tests fail. As we discussed, we can make all the required changes here. If you want to implement some of those, feel free to make a PR against my fork's main branch, which is this PR's target.

Emilios1995 added 4 commits

March 19, 2024 19:41

@Emilios1995


 Add support for the explicit partial application syntax

9af3e1f

@Emilios1995


 Support optional fields in record patterns

6651fae

@Emilios1995


 Fix error with JSX element inside pipe

e0bc632

@Emilios1995


 Support aliased inline types

e39df74

This allows adding aliased types to record fields without allowing illegal non-inline types such as record or variants.

Emilios1995

Emilios1995 commented

Mar 20, 2024

View reviewed changes

test/corpus/jsx.txt

Elements in pipes

================================================================================

<Comp />->Some

Copy link

Contributor Author

@Emilios1995 Emilios1995 Mar 20, 2024

Choose a reason for hiding this comment

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

A similar snippet was failing in the wild tests (but I forgot to add it to the list in the description.)

The problem was that pipe expressions only accept primary_expressions as their members, and JSX elements wasn't one. I fixed it by simply adding it to the list of primary_expressions. I looked at the grammar and this shouldn't have any negative repercussions.

@aspeddro

Copy link

Collaborator

aspeddro commented Mar 20, 2024

I agree with type_spread node. It's also great that we don't need to track src/parser.c

@aspeddro

Copy link

Collaborator

aspeddro commented Mar 20, 2024

What do you think about updating the highlights in another PR?

@Emilios1995


 Add separate named node for type spreads

857272e

@Emilios1995

Copy link

Contributor Author

Emilios1995 commented Mar 20, 2024

What do you think about updating the highlights in another PR?

Sounds good, I can do that.

Regarding this PR, I just merged the type_spread change, and all the wild tests are now passing.

aspeddro

aspeddro approved these changes

Mar 20, 2024

View reviewed changes

Copy link

Collaborator

@aspeddro aspeddro left a comment

Choose a reason for hiding this comment

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

Awesome work @Emilios1995 🎉

@aspeddro aspeddro merged commit 5cb02bb into rescript-lang:main

Mar 20, 2024

This was referenced Mar 20, 2024

Improve grammar installation #230

Closed

Fix infinite loop in scanner. #232

Merged

feat: dynamic module import & record type spread #233

Closed

fix: negative decimal integer literal #235

Closed

@aspeddro aspeddro mentioned this pull request

Mar 20, 2024

Fix all injections and add support for rescript-relay #237

Closed

Labels

None yet

5 participants

@Emilios1995 @aspeddro @cristianoc @zweimach @TheFedaikin

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Integrate open PRs and update instructions #239

Integrate open PRs and update instructions #239

Uh oh!

Conversation

@Emilios1995 Emilios1995 commented Mar 11, 2024 •

edited

Loading

Uh oh!

Failing wild tests

Uh oh!

Emilios1995 commented Mar 12, 2024

Uh oh!

aspeddro commented Mar 12, 2024 •

edited

Loading

Uh oh!

Uh oh!

aspeddro commented Mar 17, 2024

Uh oh!

Emilios1995 commented Mar 18, 2024

Uh oh!

Emilios1995 commented Mar 18, 2024 •

edited

Loading

Uh oh!

Uh oh!

@Emilios1995 Emilios1995 Mar 20, 2024

Choose a reason for hiding this comment

Uh oh!

aspeddro commented Mar 20, 2024

Uh oh!

aspeddro commented Mar 20, 2024

Uh oh!

Emilios1995 commented Mar 20, 2024

Uh oh!

@aspeddro aspeddro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Integrate open PRs and update instructions #239

Integrate open PRs and update instructions #239

Uh oh!

Conversation

@Emilios1995 Emilios1995 commented Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Failing wild tests

Uh oh!

Emilios1995 commented Mar 12, 2024

Uh oh!

aspeddro commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aspeddro commented Mar 17, 2024

Uh oh!

Emilios1995 commented Mar 18, 2024

Uh oh!

Emilios1995 commented Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

@Emilios1995 Emilios1995 Mar 20, 2024

Choose a reason for hiding this comment

Uh oh!

aspeddro commented Mar 20, 2024

Uh oh!

aspeddro commented Mar 20, 2024

Uh oh!

Emilios1995 commented Mar 20, 2024

Uh oh!

@aspeddro aspeddro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

@Emilios1995 Emilios1995 commented Mar 11, 2024 •

edited

Loading

aspeddro commented Mar 12, 2024 •

edited

Loading

Emilios1995 commented Mar 18, 2024 •

edited

Loading