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

Updates for v0.28.0 #55

Merged
merged 7 commits into from
Jul 29, 2023
Merged

Updates for v0.28.0 #55

merged 7 commits into from
Jul 29, 2023

Conversation

J3RN
Copy link
Member

@J3RN J3RN commented Apr 25, 2023

There's a lot here, but then again a lot changed!

Each commit is a self-contained unit, please view them individually to retain your sanity 😁

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

This all looks good! Thanks for splitting the changes into nice self-contained commits 👍

Just a minor comment on one of the tests and a thought for future unary operators

grammar.js Show resolved Hide resolved
test/highlight/expressions.gleam Outdated Show resolved Hide resolved
@J3RN
Copy link
Member Author

J3RN commented Jul 8, 2023

I wanted to update on the status of this PR. Currently it's failing the integrations suite, because some of the code therein isn't Gleam v0.28.0 compliant. There are (at least) three roads forward:

  1. Try to update the offending projects. I started this approach, invested a few hours, but ultimately didn't get very far.
  2. Remove the offending projects from our integration suite (for now, at least)
  3. Make tree-sitter-gleam backwards-compatible. I historically have versioned tree-sitter-gleam the same as Gleam versions, meaning that if you're using Gleam v0.27.0 (e.g.), then you should use tree-sitter-gleam v0.27.0. This simplified a number of things, e.g. backwards-compatibility was irrelevant and we could rename nodes to match the Gleam parser as its own nodes were renamed. It may be worthwhile reconsidering this approach, but it also entails new issues.

Feedback here welcome 😁

@the-mikedavis
Copy link
Member

I was curious about that with some of the changes from 0.30 like removing the 'external' from external types. I think it would be ok to be more permissive about deprecated / removed syntaxes. We could have some policy like keeping around old syntaxes for a few versions and eventually dropping them, since maintaining deprecated code forever is a big pain.

For the integration tests though I think we could also limit the set of repos to those that are usually up-to-date soon after syntax changes, just to minimize the maintenance burden.

@lpil
Copy link
Member

lpil commented Jul 10, 2023

Agreed! Keeping them around for at least long enough for the community to largely have moved over would be good. Currently Gleam supports both syntaxes, and I think it'd make sense for tooling to support both for longer still.

J3RN and others added 6 commits July 29, 2023 18:52
This makes me a little uneasy, as the way that the Gleam parser parses
"let assert" is that it first parses "let", then calls
"parse_assignment" which will look to see if an "assert" comes next,
and uses this information to choose the node type.

I think a closer tree-sitter implementation would look like this:

_statement: ($) => choice($._expression, $._assignment, $.use),
...
_assignment: ($) => choice($.let, $.let_assert),
let: ($) => seq("let", $._assignment2),
let_assert: ($) => seq("let", "assert", $._assignment2)
_assignment2: ($) =>
  ...

However this requires an awkward "_assignment2" and isn't
substantially closer to the Gleam parser to warrant the change.

Also we could simply do `optional("assert")`, but that would make
"let" and "let assert" the same node type, which I suspect is an anti-feature.
Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
@J3RN J3RN merged commit 25f5d36 into gleam-lang:main Jul 29, 2023
1 of 2 checks passed
@J3RN J3RN deleted the updates-for-0.28.0 branch July 29, 2023 23:00
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

3 participants