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

Add linter to packages/ folder #18885

Merged
merged 9 commits into from Feb 25, 2019

Conversation

Projects
None yet
4 participants
@rafeca
Copy link
Contributor

commented Feb 21, 2019

Requirements for Contributing a Bug Fix

Identify the Bug

Linter was not checking the packages/ folder.

Description of the Change

The JS code style of the packages/ folder is not matching the style of the rest of the repository, this is caused by not having the linter running on this folder.

This PR enables the linter and fixes all the linting issues that appeared on that folder. Most of the issues have been fixed automatically by running prettier-standard on that folder, the things that needed to be fixed manually are in separate commits.

Alternate Designs

N/A

Possible Drawbacks

This PR changes a bunch of files, and while prettier should only do style changes, there's always the possibility of breaking something when making big changes. To mitigate this risk I'm gonna review some of the changes manually and make sure that they're ok.

Verification Process

I'm going to manually verify the automatic fixes and rely on the existing automated tests.

Release Notes

N/A (it's only an internal change)

@rafeca rafeca marked this pull request as ready for review Feb 21, 2019

@rafeca rafeca force-pushed the rafeca:add-linting-to-packages branch from 8de9193 to f5cc327 Feb 21, 2019


// the following regular expression is executed natively via the `substring` package,
// where `\A` corresponds to the beginning of the string.
// eslint-disable-next-line no-useless-escape

This comment has been minimized.

Copy link
@rafeca

rafeca Feb 21, 2019

Author Contributor

@jasonrudolph, @smashwilson can you verify if what I wrote here is correct? 🙃

This comment has been minimized.

Copy link
@jasonrudolph

jasonrudolph Feb 22, 2019

Member

@rafeca: That sounds mostly right. I see that the \A was introduced in atom/line-ending-selector#56. The explanation in that PR might help you craft the comment above and/or you could consider including a link to that PR. Does that help?

This comment has been minimized.

Copy link
@rafeca

rafeca Feb 23, 2019

Author Contributor

Yes, thank you!

@Arcanemagus
Copy link
Contributor

left a comment

Thanks for starting this! Always great to see more code brought under enforcement of the coding style/validity checking.

@@ -14,7 +14,7 @@ export default class StatusIconComponent {
render () {
return (
<div className='incompatible-packages-status inline-block text text-error'>
<span className='icon icon-bug'></span>
<span className='icon icon-bug' />

This comment has been minimized.

Copy link
@Arcanemagus

Arcanemagus Feb 21, 2019

Contributor

The ending </span> isn't allowed to be omitted in valid HTML, is etch fixing this or are we just relying on Chrome to fix it for us (which should be fixed)?

This comment has been minimized.

Copy link
@rafeca

rafeca Feb 21, 2019

Author Contributor

Good point! in this case I'm pretty sure that this code gets transpiled (probably by babel) into etch calls to create DOM elements, since this is standard JSX.

Probably @nathansobo or @as-cii know better :)

@rafeca rafeca changed the title Add linting to packages Add linter to packages/ folder Feb 22, 2019

@jasonrudolph
Copy link
Member

left a comment

This PR changes a bunch of files, and while prettier should only do style changes, there's always the possibility of breaking something when making big changes. To mitigate this risk I'm gonna review some of the changes manually and make sure that they're ok.automated tests.

I can't claim to have reviewed every line, but as noted in the PR body, "prettier should only do style changes," so this should be a safe change to merge. And since you're planning to review the non-automatic changes manually to make sure that they're OK, I think this is safe to ship once you've completed your verification.

@rafeca rafeca force-pushed the rafeca:add-linting-to-packages branch 2 times, most recently from 7938bb5 to e00d2b3 Feb 23, 2019

@rafeca rafeca force-pushed the rafeca:add-linting-to-packages branch from e00d2b3 to 034a05c Feb 25, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

I've reviewed the automatic changes done by prettier everything I found is safe:

  • Whitespace changes to avoid long lines.
  • Use single quotes for strings whenever it's possible.
  • Remove semicolons when it's safe.
  • Does not add trailing commas on objects or functions, so it should be safe on old JS engines.

The failing test is related to watchPath. Since that code was not changed in this PR I'm assuming that the error is not related and that that test is flaky (I've seen it failing on other builds).

@rafeca rafeca merged commit b2c0028 into atom:master Feb 25, 2019

0 of 3 checks passed

Atom Pull Requests in progress
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@rafeca rafeca deleted the rafeca:add-linting-to-packages branch Feb 25, 2019

@rafeca rafeca referenced this pull request Feb 25, 2019

Merged

Fix git-diff package test #18906

@smashwilson

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

The failing test is related to watchPath. Since that code was not changed in this PR I'm assuming that the error is not related and that that test is flaky (I've seen it failing on other builds).

👍 Yeah, I'm (slowly) investigating that one in #18857.

@rafeca rafeca self-assigned this Feb 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.