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.

Sign up for GitHub

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

Jump to bottom

Prepare for bumping ESLint to 4.6.1 #17868

Merged
merged 2 commits into from
Sep 16, 2017
Merged

Prepare for bumping ESLint to 4.6.1 #17868

merged 2 commits into from
Sep 16, 2017

Conversation

oandregal
Copy link
Contributor

@oandregal oandregal commented Sep 6, 2017 •
edited
Loading

Prepare for bumping ESLint version to 4.6.1.

Testing

Check that CircleCI test PRs run as expected

Locally:

  • Clone this branch.
  • Add some changes that break some lint rule (remove spaces, for example), and commit: the expected result is that the git commit hook reports the linting error and prevents you from committing them.
  • You may want to commit them anyway (use --no-verify) and push to a remote branch to double-check that CircleCI reports the same.
  • Repeat the experiment but with changes that don't break any linting rule (for example, remove the debug statements in some file). The expected result is that the git commit hook doesn't report any linting error and allows you to commit them.
  • If you push them to a remote branch, CircleCI shouldn't report any linting issues.

@matticbot matticbot added the [Size] S Small sized issue label Sep 6, 2017
@oandregal oandregal self-assigned this Sep 6, 2017
@oandregal oandregal requested a review from aduth September 6, 2017 19:12
@oandregal oandregal mentioned this pull request Sep 6, 2017
Closed
@oandregal oandregal mentioned this pull request Sep 7, 2017
Closed
@oandregal
Copy link
Contributor Author

Jarda, I cannot test the prettier magic. Would you mind giving this PR a try and confirm that it doesn't break anything for prettier?

@oandregal oandregal mentioned this pull request Sep 7, 2017
Closed
@jsnajdr
Copy link
Member

jsnajdr commented Sep 7, 2017

The Prettier magic can be tested by anyone, no editor integration setup is needed. Here's what I did to verify that this branch is still OK:

  1. Find some file that already has the @format tag.
  2. Insert a ternary with nested objects somewhere, formatted exactly like this:
const x = true
  ? {
      a: 1
    }
  : {
      b: 1
    };

That's how Prettier would format it. indent rule prefers something else:

const x = true
  ? {
    a: 1
  }
  : {
    b: 1
  };

But we want to ignore the indent rule rather than make it happy.
3. Try to commit this change on a branch. Because the @format tag is present, the pre-commit hook won't complain about indent rule breaks and the commit is successful.
4. Undo the commit with git reset HEAD^
5. Remove the @format tag at the start of the file and commit again.
6. This time, the pre-commit hook should fail. There's an eslint error that is not ignored!

I verified that this branch still does the right magic! 👍

@aduth aduth mentioned this pull request Sep 7, 2017
Merged
@matticbot matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] S Small sized issue labels Sep 7, 2017
aduth
aduth reviewed Sep 7, 2017
View reviewed changes
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Updated the Calypso ESLint dependencies to their now-released versions (1.0.0 config, 4.0.0 plugin). Also included necessary shrinkwrap revisions.

Will want to point ESLines to their npm releases prior to merge (and update shrinkwrap accordingly) but otherwise this looks good 👍

@oandregal
Copy link
Contributor Author

@jsnajdr @aduth Upon rebasing master I ended up having to fix some no-unused-rules that were in master. Would you mind reviewing that specifically? Otherwise, eslint-eslines was released and this is ready for merging.

@oandregal oandregal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Sep 8, 2017
aduth
aduth suggested changes Sep 8, 2017
View reviewed changes
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Glad to see ESLint becoming smarter and catching more issues 😄

There's quite a few removals in the shrinkwrap that I wouldn't have expected, and in fact are added back when regenerate locally. Did you generate the shrinkwrap with npm run update-deps?

client/state/http/actions.js Outdated
@@ -26,7 +26,6 @@ export const http = ( {
url,
method,
headers,
queryParams,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I think the mistake here was that it should have been included in the generated action return object, since it's later expected in the HTTP handler:

wp-calypso/client/state/http/index.js

Line 39 in a40fa96

queryParams = [],

cc @yurynix @dmsnell (#16036, d2dff95).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense - added it back, solved the issue, and updated the docs. Yuri, Dennis: would you mind confirming that's what you intended and provide testing instructions?

Copy link
Contributor

Choose a reason for hiding this comment

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

The tests @nosolosw added looks good.
There isn't anything that uses queryParams for the generic http handler afaik, so the only tests are unit tests.

@oandregal
Copy link
Contributor Author

Yes, I used npm run update-deps. I know that fsevents and its dependencies are removed every time a linux user (like myself) generates the shrinkwrap because that's only needed in MacOS. Do you see any other cases?

@aduth
Copy link
Contributor

aduth commented Sep 8, 2017

From what I was seeing, most everything removed in the changes here was added back.

@oandregal
Copy link
Contributor Author

oandregal commented Sep 8, 2017 •
edited
Loading

Pushed the changes suggested by Andrew and re-created the shrinkwrap.

oandregal
oandregal commented Sep 15, 2017
View reviewed changes
client/extensions/woocommerce/state/ui/shipping/zones/locations/selectors.js
@@ -543,7 +543,7 @@ export const getShippingZoneLocationsList = createSelector(
const locations = getShippingZoneLocations( state, zoneId, siteId );
return getShippingZoneLocationsListFromLocations( state, locations, maxCountries, siteId );
},
( state, zoneId, maxCountries = 999, siteId = getSelectedSiteId( state ) ) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that i needed to remove this initialization for the no-unused-vars not to fail.

oandregal
oandregal commented Sep 15, 2017
View reviewed changes
client/extensions/woocommerce/state/ui/shipping/zones/locations/selectors.js
@@ -567,7 +567,7 @@ export const getCurrentlyEditingShippingZoneLocationsList = createSelector(
const locations = getShippingZoneLocationsWithEdits( state, siteId, false );
return getShippingZoneLocationsListFromLocations( state, locations, maxCountries, siteId );
},
( state, maxCountries = 999, siteId = getSelectedSiteId( state ) ) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that i needed to remove this initialization for the no-unused-vars not to fail.

oandregal
oandregal commented Sep 15, 2017
View reviewed changes
client/state/signup/optional-dependencies/reducer.js
@@ -9,7 +9,7 @@ import { suggestedUsernameSchema } from './schema';

const suggestedUsername = createReducer( '',
{
[ SIGNUP_OPTIONAL_DEPENDENCY_SUGGESTED_USERNAME_SET ]: ( state = null, action ) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that i needed to remove this initialization for the no-unused-vars not to fail.

oandregal
oandregal commented Sep 15, 2017
View reviewed changes
client/state/signup/steps/design-type/reducer.js
@@ -11,7 +11,7 @@ import { designTypeSchema } from './schema';

export default createReducer( '',
{
[ SIGNUP_STEPS_DESIGN_TYPE_SET ]: ( state = '', action ) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that i needed to remove this initialization for the no-unused-vars not to fail.

oandregal
oandregal commented Sep 15, 2017
View reviewed changes
client/state/signup/steps/site-title/reducer.js
@@ -11,7 +11,7 @@ import { siteTitleSchema } from './schema';

export default createReducer( '',
{
[ SIGNUP_STEPS_SITE_TITLE_SET ]: ( state = '', action ) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that i needed to remove this initialization for the no-unused-vars not to fail.

oandregal
oandregal commented Sep 15, 2017
View reviewed changes
client/state/ui/editor/video-editor/reducer.js
@@ -46,7 +46,7 @@ export const uploadProgress = ( state = null, { type, percentage } ) => {
* @param {Object} action Action object
* @return {Boolean} Whether or not an error should now be shown
*/
export const showError = ( state = false, { type } ) => type === VIDEO_EDITOR_SHOW_ERROR;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that i needed to remove this initialization for the no-unused-vars not to fail.

jsnajdr
jsnajdr reviewed Sep 16, 2017
View reviewed changes
client/state/data-layer/wpcom/activity-log/rewind/restore-status/index.js Outdated
@@ -50,7 +50,7 @@ export const receiveRestoreProgress = ( { dispatch }, { siteId, timestamp, resto
};

// FIXME: Could be a network Error (instanceof Error) or an API error. Handle each case correctly.
export const receiveRestoreError = ( { dispatch }, { siteId, timestamp, restoreId }, error ) => {
export const receiveRestoreError = ( { dispatch }, {}, error ) => {
Copy link
Member

Choose a reason for hiding this comment

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

I didn't know it's possible to specify an empty destructuring ;) It's better to name the parameter:

( { dispatch }, action, error ) => ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Automattic motto: always learning! :D Thanks for catching this, it's fixed now.

jsnajdr
jsnajdr reviewed Sep 16, 2017
View reviewed changes
client/state/http/actions.js
@@ -37,6 +38,7 @@ export const http = ( {
url,
method,
headers,
queryParams,
Copy link
Member

Choose a reason for hiding this comment

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

Nice bugfix here 👍

jsnajdr
jsnajdr approved these changes Sep 16, 2017
View reviewed changes
Copy link
Member

@jsnajdr jsnajdr left a comment

Choose a reason for hiding this comment

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

I'd recommend to do the {} => action change. Otherwise, this looks 👍

@oandregal oandregal merged commit e53c91b into master Sep 16, 2017
@oandregal oandregal deleted the update/eslint-4 branch September 16, 2017 04:31
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 16, 2017
@oandregal oandregal mentioned this pull request Sep 17, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurynix yurynix yurynix left review comments

@aduth aduth aduth requested changes

@jsnajdr jsnajdr jsnajdr approved these changes

Assignees

@oandregal oandregal

Labels
[Size] XL Probably needs to be broken down into multiple smaller issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@oandregal @jsnajdr @aduth @yurynix @matticbot