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

Avoid some errors that break eslint-plugin-ember for gts #1667

Closed
wants to merge 1 commit into from

Conversation

gitKrystan
Copy link

@gitKrystan gitKrystan commented Nov 17, 2022

I needed these changes to get eslint-plugin-ember running on my gts files.

@bmish
Copy link
Member

bmish commented Nov 17, 2022

@NullVoxPopuli @nlfurniss?

@NullVoxPopuli
Copy link
Contributor

I like the branch name 🙃

@@ -105,7 +105,8 @@ function mapRange(messages, filename) {

return flattened.map((message) => {
const line = lines[message.line - 1];
const token = line.slice(message.column - 1, message.endColumn - 1);
// `line` can be undefined here sometimes. I suspect maybe `message.line` is incorrect above.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this is due to file-wide messages, or maybe there is another property to use?
Anywho, happy to see this more resilient

Copy link
Member

Choose a reason for hiding this comment

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

I think we need to figure out whether this is a legitimate scenario. I would rather we fail fast on unexpected/invalid input than hide potential problems. And of course, if it is a legitimate scenario, need to have tests for it.

Copy link
Author

Choose a reason for hiding this comment

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

FWIW line numbers are way off for most of my error messages (usually 0:0). The fixes in this branch only get things running.

Copy link
Contributor

Choose a reason for hiding this comment

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

turns out this is a legit scenario, because folks can cause parse errors, which occur on the whole file and don't have a line/column/etc

</template>
`,
},
// This test fails on master with "Preprocessing error: Cannot overwrite a zero-length range – use appendLeft or prependRight instead"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, so we need to figure this out, still 🤔

Copy link
Member

Choose a reason for hiding this comment

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

@NullVoxPopuli @gitKrystan we need to get master unblocked. Can one of you submit a PR to fix or disable this test? I can't accept the current PR as-is because it has untested changes in it. It may be best to fix the test on master first and then work on the bug fix afterward.

FAIL tests/lib/rules-preprocessor/gjs-gts-processor-test.js
  ● template-vars › valid › 
      const Foo = <template>hi</template>

      <template>
        <Foo />
      </template>
    

    expect(received).toBe(expected) // Object.is equality

    Expected: ""
    Received: "Preprocessing error: Cannot overwrite a zero-length range – use appendLeft or prependRight instead"

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure I have enough context to get this to a mergeable state, but I'd be happy to pair with someone on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

One of these commits is causing the error: ember-cli/ember-template-imports@v3.1.1...v3.1.2

I'm about to put up a PR fixing the test

Copy link
Contributor

Choose a reason for hiding this comment

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

@bmish because this feature is new, I'd be fine with leaving the test commented out for now -- there are a couple things with the position mapping in general that might need more in-depth looking, I think

Copy link
Member

Choose a reason for hiding this comment

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

Skipping the test in a separate PR is fine as a temporary solution since it's blocking master.

@bmish
Copy link
Member

bmish commented Aug 22, 2023

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