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

Improve report location calculations #7

Closed
gustavnikolaj opened this issue Mar 20, 2017 · 5 comments
Closed

Improve report location calculations #7

gustavnikolaj opened this issue Mar 20, 2017 · 5 comments

Comments

@gustavnikolaj
Copy link

Given this example:

import expect from '../../../../testUtils/unexpectedReact';
import React from 'react';
import FormattedSize from '../FormattedSize';

it('should render 123 bytes as 0.1KB', () => expect(
    <FormattedSize size={123} />,
    'to render as',
    <span>0.1 KB</span>
));

it('should render 1241240 bytes as 1.2MB', () => expect(
    <FormattedSize size={1241240} />,
    'to render as',
    <span>1.2 MB</span>
));

Prettier wants it formatted as:

import expect from '../../../../testUtils/unexpectedReact';
import React from 'react';
import FormattedSize from '../FormattedSize';

it('should render 123 bytes as 0.1KB', () =>
    expect(<FormattedSize size={123} />, 'to render as', <span>0.1 KB</span>));

it('should render 1241240 bytes as 1.2MB', () =>
    expect(
        <FormattedSize size={1241240} />,
        'to render as',
        <span>1.2 MB</span>
    ));

But running eslint gives me the following report:

$ ./node_modules/.bin/eslint src/common/components/FormattedSize/__tests__/FormattedSize-test.js 

./src/common/components/FormattedSize/__tests__/FormattedSize-test.js
  5:45  error  Follow `prettier` formatting (expected '\n' but found ' ')  prettier/prettier

I expected an error to be reported on both line 5 and 11, not just line 5. If I proceed to actually run prettier, both are fixed up as expected.

Other rules continue to work as expected, but this plugin and it's rule will only report the first problem.

@not-an-aardvark
Copy link
Member

This is working as intended, although there is some room for improvement. The plugin just compares the original text to the prettier-formatted text, and reports an error at the first location where the two strings differ. At the moment, it doesn't generate line-by-line diffs.

I'm open to a PR to compare the two strings more intelligently.

@not-an-aardvark not-an-aardvark changed the title Only reporting the first problem of many Improve report location calculations Mar 21, 2017
@gustavnikolaj
Copy link
Author

I don't see how the location problem is related to my initial report 😄

I think the wording makes me expect that it is an exhaustive report that I get, and it is not in sync with my expectations as a user, that I get an error report worded like that, and only one of them.

I realize why it's hard to do anything about, but an alternative solution that wouldn't cause this user experience WTF moment could be to just have it fail with an error on line 1 character 1:

This file contains code that is different to the output of prettier.

@not-an-aardvark
Copy link
Member

Hmm, I'm not sure I want to just output an error on line 1 character 1, because in some cases maybe the error message is useful (e.g. if someone is missing a semicolon). Making the error message clearer seems like it could be a good idea though.

@zertosh
Copy link
Member

zertosh commented May 15, 2017

This will be fixed once #21 is merged.

@zertosh
Copy link
Member

zertosh commented May 17, 2017

I just published eslint-plugin-prettier@2.1.0 which fixes this

@zertosh zertosh closed this as completed May 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants