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

Remove postcss-reporter package #4858

Merged
merged 3 commits into from Jul 17, 2020
Merged

Remove postcss-reporter package #4858

merged 3 commits into from Jul 17, 2020

Conversation

ybiquitous
Copy link
Member

@ybiquitous ybiquitous commented Jul 8, 2020

Which issue, if any, is this issue related to?

Related to #2454

Is there anything in the PR that needs further explanation?

This change removes the postcss-reporter package and reduces the dependencies.
As you see the changed package-lock.json, this package depends on:

  • chalk
  • lodash
  • log-symbols
  • postcss

See also #2454 (comment)

We've used only the tiny utility function getLocation() in the package.
This function just returns an object with the line and column property in our project, so I think we can replace it with a simple code.

https://github.com/postcss/postcss-reporter/blob/f01a601ea2cd41d626e561969d66a765b3afcb2d/lib/util.js#L4-L13

This change removes the `postcss-reporter` package and reduces the dependencies.
As you see the changed `package-lock.json`, this package depends on:

- `chalk`
- `lodash`
- `log-symbols`
- `postcss`

See also #2454 (comment)

We've used only the tiny utility function `getLocation()` in the package.
This function just returns an object with the `line` and `column` property in our project,
so I think we can replace it with a simple code.

https://github.com/postcss/postcss-reporter/blob/f01a601ea2cd41d626e561969d66a765b3afcb2d/lib/util.js#L4-L13
Comment on lines +142 to +143
line ? line.toString() : '',
column ? column.toString() : '',
Copy link
Member Author

@ybiquitous ybiquitous Jul 8, 2020

Choose a reason for hiding this comment

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

[note] The message variable is a StylelintWarning, so checking null seems needless:

export type StylelintWarning = {
line: number;
column: number;

Suggested change
line ? line.toString() : '',
column ? column.toString() : '',
line.toString(),
column.toString(),

But, if changing it, the following test fails:

 FAIL  lib/formatters/__tests__/verboseFormatter.test.js
  stringFormatter
    ✓ outputs no warnings (3 ms)
    ✓ outputs one warnings (of severity 'error') (6 ms)
    ✓ outputs 0 stdout column (1 ms)
    ✓ outputs less than 80 stdout column (1 ms)
    ✓ outputs two of the same warnings of 'error' and one of 'warning' across two files (3 ms)
    ✕ outputs lineless syntax error
    ✓ outputs one ignored file

  ● stringFormatter › outputs lineless syntax error

    TypeError: Cannot read property 'toString' of undefined

      140 | 		 */
      141 | 		const row = [
    > 142 | 			line.toString(),
          | 			     ^
      143 | 			column.toString(),
      144 | 			symbols[severity]
      145 | 				? chalk[/** @type {'blue' | 'red' | 'yellow'} */ (levelColors[severity])](symbols[severity])

In this case, the failed test case seems wrong (should have lines and columns?), but I'm not confident whether I should fix it: 😓

warnings: [
{
rule: 'SyntaxError',
severity: 'error',
text: 'Unexpected foo',
},
],

@ybiquitous ybiquitous marked this pull request as ready for review July 8, 2020 16:52
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.

In this case, the failed test case seems wrong (should have lines and columns?), but I'm not confident whether I should fix it

Perhaps we leave for now and revisit.

Copy link
Member

@m-allanson m-allanson left a comment

Choose a reason for hiding this comment

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

Nice!

In this case, the failed test case seems wrong

I agree, the test case seems wrong. I also agree with @jeddy3, let's leave it in for now.

@ybiquitous
Copy link
Member Author

@jeddy3 @m-allanson Thanks for the review. As you said, it seems good to leave it now. 👍

@m-allanson m-allanson merged commit cecacb9 into stylelint:master Jul 17, 2020
@ybiquitous ybiquitous deleted the remove-postcss-reporter branch July 17, 2020 12:37
m-allanson added a commit that referenced this pull request Jul 23, 2020
* master:
  Update CHANGELOG.md
  Fix false negatives for where, is, nth-child and nth-last-child in selector-max-* (except selector-max-type) (#4842)
  Bump @types/lodash from 4.14.155 to 4.14.157 (#4869)
  Remove `postcss-reporter` package (#4858)
  Bump lodash from 4.17.15 to 4.17.19 (#4864)
  Replace 3rd-party type definitions (#4857)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants