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

Upgrade: eslint & eslint-config-eslint #387

Merged
merged 13 commits into from
Nov 4, 2018
Merged

Upgrade: eslint & eslint-config-eslint #387

merged 13 commits into from
Nov 4, 2018

Conversation

aladdin-add
Copy link
Member

@aladdin-add aladdin-add commented Jul 6, 2018 •
edited
Loading

the pr upgrade these devdeps:

  • "eslint": "^5.6.1",
  • "eslint-config-eslint": "^5.0.0",
  • "eslint-plugin-node": "^7.0.1",
  • "istanbul": "^0.4.5",

platinumazure
platinumazure reviewed Jul 6, 2018
View reviewed changes
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

Is no-unused-vars enabled? If not, should we enable it in this codebase?

@aladdin-add
Copy link
Member Author

that's what I am thinking, will do.

@aladdin-add
Copy link
Member Author

it would be fine to add eslint-plugin-node. I added it in eslint-config-eslint, but it has not been released yet. Another choice is moving espree to eslint repo (monorepo). I created an issue (eslint/eslint#10575).

@aladdin-add aladdin-add changed the title Chore: rm some unused vars Upgrade: eslint & eslint-config-eslint Jul 9, 2018
platinumazure
platinumazure reviewed Jul 9, 2018
View reviewed changes
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

Why the change from Chai Assert to Node's assert library? Chai Assert usually gives much more expression assertion messages and works nicely with Mocha.

@mysticatea
Copy link
Member

I feel built-in assert is nice enough :D

@aladdin-add aladdin-add force-pushed the chore/fix-alert branch 3 times, most recently from 02a9a34 to 8f027ae Compare September 30, 2018 06:49
@aladdin-add aladdin-add mentioned this pull request Oct 10, 2018
Merged
@kaicataldo
Copy link
Member

Do we want to wait on #391 to land before doing this work? Looks like it might caused some merge conflicts.

@aladdin-add aladdin-add force-pushed the chore/fix-alert branch 2 times, most recently from 1a7fbac to b428439 Compare October 18, 2018 04:47
@aladdin-add
Copy link
Member Author

sure, I like to see istanbul => nyc (it has been done in #391 )

@kaicataldo
Copy link
Member

#391 has been merged!

@aladdin-add
Copy link
Member Author

@kaicataldo I just rebased the newest changes, and thanks for the reminder! :)

kaicataldo
kaicataldo reviewed Oct 27, 2018
View reviewed changes
Makefile.js
* @returns {Function} The function to pass into a filter method.
* @private
*/
function fileType(extension) {
Copy link
Member

Choose a reason for hiding this comment

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

Was this unused?

Copy link
Member Author

Choose a reason for hiding this comment

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

it was used before the change, but I replaced it with lib/*.js.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see - missed that. Thanks 👍

kaicataldo
kaicataldo reviewed Oct 27, 2018
View reviewed changes
Makefile.js Outdated
@@ -112,21 +94,21 @@ target.browserify = function() {
// 1. create temp and build directory
if (!test("-d", TEMP_DIR)) {
mkdir(TEMP_DIR);
mkdir(TEMP_DIR + "/lib");
mkdir(`${TEMP_DIR}/lib`);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the / is unnecessary here. I wonder if we should just use path.join()?

kaicataldo
kaicataldo reviewed Oct 27, 2018
View reviewed changes
Makefile.js Outdated
}

if (!test("-d", BUILD_DIR)) {
mkdir(BUILD_DIR);
}

// 2. copy files into temp directory
cp("-r", "lib/*", TEMP_DIR + "/lib");
cp("-r", "lib/*", `${TEMP_DIR}/lib`);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the / is unnecessary here. I wonder if we should just use path.join()?

kaicataldo
kaicataldo reviewed Oct 27, 2018
View reviewed changes
Makefile.js Outdated
cp("espree.js", TEMP_DIR);
cp("package.json", TEMP_DIR);


// 3. browserify the temp directory
nodeCLI.exec("browserify", TEMP_DIR + "espree.js", "-o", BUILD_DIR + "espree.js", "-s espree");
nodeCLI.exec("browserify", `${TEMP_DIR}espree.js`, "-o", `${BUILD_DIR}espree.js`, "-s espree");
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should just use path.join()?

kaicataldo
kaicataldo approved these changes Oct 27, 2018
View reviewed changes
Copy link
Member

@kaicataldo kaicataldo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, with a few very minor (non-blocking) suggestions.

I didn't realize we were also using assert in the main ESLint repo. Just so I'm clear, are we trying to move towards using Node's assert module?

@aladdin-add
Chore: use path.join
e4cfca1 
Signed-off-by: weiran.zsd <weiran.zsd@alibaba-inc.com>
@aladdin-add
Copy link
Member Author

are we trying to move towards using Node's assert module?

no, I don't think we have made the decision.

@aladdin-add aladdin-add merged commit 6ebc219 into eslint:master Nov 4, 2018
@aladdin-add aladdin-add deleted the chore/fix-alert branch November 4, 2018 15:18
matz3 added a commit to matz3/espree that referenced this pull request Jul 9, 2021
@matz3
Chore: Remove obsolete lib/visitor-keys.js
acb0bce 
PR eslint#389 deleted "lib/visitor-keys.js" and replaced all usages with the
eslint-visitor-keys package.
PR eslint#387 added lib/visitor-keys.js back again for no apparent reason.

It looks like eslint#387 has been started before eslint#389, so it needed to
integrate those changes and by mistake the file has been added again.
@matz3 matz3 mentioned this pull request Jul 9, 2021
Merged
aladdin-add pushed a commit that referenced this pull request Jul 9, 2021
@matz3
Chore: Remove obsolete lib/visitor-keys.js (#509)
bf57077 
PR #389 deleted "lib/visitor-keys.js" and replaced all usages with the
eslint-visitor-keys package.
PR #387 added lib/visitor-keys.js back again for no apparent reason.

It looks like #387 has been started before #389, so it needed to
integrate those changes and by mistake the file has been added again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@platinumazure platinumazure platinumazure left review comments

@kaicataldo kaicataldo kaicataldo approved these changes

Assignees
No one assigned
Labels
chore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aladdin-add @mysticatea @kaicataldo @platinumazure