Skip to content

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

Merged
merged 14 commits into from
Mar 20, 2024

Conversation

Emilios1995
Copy link
Contributor

@Emilios1995 Emilios1995 commented Mar 11, 2024

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, ...))

Sorry, something went wrong.

@Emilios1995
Copy link
Contributor Author

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

#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>
}

@aspeddro
Copy link
Collaborator

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

@Emilios1995
Copy link
Contributor Author

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

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:

  1. 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.
  2. 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.

Elements in pipes
================================================================================

<Comp />->Some
Copy link
Contributor Author

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

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

@aspeddro
Copy link
Collaborator

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

@Emilios1995
Copy link
Contributor Author

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.

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 🎉

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

5 participants