Skip to content

Fix end line comment not being included in node text #144

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 6 commits into from
Dec 18, 2023

Conversation

Qluxzz
Copy link
Contributor

@Qluxzz Qluxzz commented Dec 15, 2023

type Foo
    = Bar -- This is a Bar

Currently when hovering over Foo in VS Code the result is this:

type Foo
    = Bar

The ending line comment is dropped.

However for types with multiple variants, such as the following:

type Foo
    = Bar -- This is a Bar
    | Biz -- This is a Biz

The result when hovering is the following:

type Foo
    = Bar -- This is a Bar
    | Biz

The first variant's line comment is included, but not the last one.

The following changes makes it so the ending line comment is included in the correct node text.

I've also verified that these changes have the correct behaviour in the elm-language-server repo with the following tests added to test/hoverProvider.test.ts and using a locally built tree-sitter-elm.wasm version:

  it("should include line comment when hovering", async () => {
    const source = `
--@ Test.elm
module Test exposing (..)

type Foo
    = Bar -- This is a Bar


bar : Foo
     --^
bar = Bar
  `;

    await testHover(
      source,
      "```elm\ntype Foo\n    = Bar -- This is a Bar\n```",
    );
  });

  it("should include end line comment when hovering", async () => {
    const source = `
  --@ Test.elm
module Test exposing (..)

type Foo
    = Bar -- This is a Bar
    | Biz -- This is a Biz


bar : Foo
     --^
bar = Bar
  `;

    await testHover(
      source,
      "```elm\ntype Foo\n    = Bar -- This is a Bar\n    | Biz -- This is a Biz\n```",
    );
  });

@razzeee
Copy link
Member

razzeee commented Dec 16, 2023

Thank you, seems sane to me. I will probably squash this when merging.

@Qluxzz
Copy link
Contributor Author

Qluxzz commented Dec 16, 2023

Yeah, go right ahead and squash, I naively assumed that squashing was the default merging strategy.

@razzeee
Copy link
Member

razzeee commented Dec 17, 2023

Unfortunatly this regresses a bit

With your MR

Total parses: 20011; successful parses: 19930; failed parses: 81; success percentage: 99.60% 

Main branch

Total parses: 20011; successful parses: 19938; failed parses: 73; success percentage: 99.64% 

I guess we should check those 8 cases

@Qluxzz
Copy link
Contributor Author

Qluxzz commented Dec 17, 2023

I diffed the log files and these are the new regressions.

examples-full/Janiczek/elm-graph/src/Graph.elm                           1 ms  (ERROR [70, 8] - [77, 9])
examples-full/Kurren123/k-dropdown-container/src/DropdownContainer.elm   0 ms  (ERROR [63, 13] - [63, 16])
examples-full/MacCASOutreach/graphicsvg/src/GraphicSVG.elm               11 ms (ERROR [514, 4] - [514, 7])
examples-full/arowM/elm-neat-layout/src/Neat/Internal.elm                1 ms  (ERROR [64, 4] - [64, 7])
examples-full/evancz/elm-playground/src/Playground.elm                   4 ms  (ERROR [828, 2] - [828, 5])
examples-full/folkertdev/elm-brotli/src/RingBuffer.elm                   1 ms  (ERROR [19, 21] - [19, 24])
examples-full/jeongoon/elmnt-scrollpicker/src/Elmnt/BaseScrollPicker.elm 6 ms  (ERROR [248, 32] - [248, 47])
examples-full/pfcoperez/elm-playground/src/Playground.elm                4 ms  (ERROR [828, 2] - [828, 5])

@Qluxzz
Copy link
Contributor Author

Qluxzz commented Dec 17, 2023

I compared the new regressions to the main branch and these new tests catch the regression:

================================================================================
Type declaration with union variant and associated data and line comment on new line
================================================================================

type Foo
    = Bar
        -- First associated data
        Int

--------------------------------------------------------------------------------

(file
      (type_declaration
        (type)
        (upper_case_identifier)
        (eq)
        (union_variant
          (upper_case_identifier)
          (line_comment)
          (type_ref
            (upper_case_qid
              (upper_case_identifier))))))

================================================================================
Type declaration with union variant and associated data with line comment on same line
================================================================================

type Foo
    = Bar
        Int -- First associated data
        Float -- Second associated data

--------------------------------------------------------------------------------

(file
      (type_declaration
        (type)
        (upper_case_identifier)
        (eq)
        (union_variant
          (upper_case_identifier)
          (type_ref
            (upper_case_qid
              (upper_case_identifier)))
          (line_comment)
          (type_ref
            (upper_case_qid
              (upper_case_identifier))))
          (line_comment)))

I'm currently working on a fix.

@Qluxzz
Copy link
Contributor Author

Qluxzz commented Dec 18, 2023

Running npm run test-full with the latest changes does now not report any regressions, compared to the main branch.

Total parses: 20011; successful parses: 19938; failed parses: 73; success percentage: 99.64%

@razzeee
Copy link
Member

razzeee commented Dec 18, 2023

LGTM, let's see if this still works in the server

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