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

refactor(convention): using es-next conventions #200

Merged
merged 3 commits into from Feb 22, 2017

Conversation

Projects
None yet
6 participants
@sarbbottam
Copy link
Owner

sarbbottam commented Oct 30, 2016

.travis.yml Outdated
@@ -7,12 +7,8 @@ notifications:
email: false
node_js:
- stable

This comment has been minimized.

@ta2edchimp

ta2edchimp Oct 30, 2016

Collaborator

Should we explicitly test on current lts, too? There may be issues with latest/7.0.0 concerning graceful-fs.

This comment has been minimized.

@sarbbottam
@ta2edchimp
Copy link
Collaborator

ta2edchimp left a comment

On the overall, we should really keep an eye on the code's readability during "es-nextification".

"scripts": {
"cover": "nyc --reporter=lcov --reporter=text npm test",
"test": "mocha --recursive",
"lint": "eslint --ignore-pattern test/fixtures .",
"test": "xo && mocha --recursive",
"update-contributors": "all-contributors generate",
"commit": "git-cz",
"validate": "npm-run-all --parallel lint cover --sequential check-coverage",

This comment has been minimized.

@ta2edchimp

ta2edchimp Oct 30, 2016

Collaborator

lint should be either replaced with test or the separation of the test and lint npm scripts should be kept.
I'd prefer the latter.

"report-coverage": "cat ./coverage/lcov.info | node_modules/.bin/codecov",
"semantic-release": "semantic-release pre && npm publish && semantic-release post",
"travis-after-all": "travis-after-all && npm run report-coverage && npm run semantic-release"
},
"files": ["dist"],
"bin": {
"eslint-find-rules": "src/bin/find.js",
"eslint-diff-rules": "src/bin/diff.js"

This comment has been minimized.

@ta2edchimp

ta2edchimp Oct 30, 2016

Collaborator

bin script paths have to be adjusted to the dist output path accordingly.

@@ -37,11 +38,13 @@
},
"devDependencies": {
"all-contributors-cli": "3.0.6",
"babel-cli": "^6.18.0",
"babel-core": "^6.18.0",
"babel-preset-es2015": "^6.18.0",

This comment has been minimized.

@ta2edchimp

ta2edchimp Oct 30, 2016

Collaborator

Personally, I prefer --save-exact/ -E and to depend upon fixed dependencies' versions to reduce possible side effects that may appear with upcoming versions.

})
cli.push(rules, 2, false)
rules = rules.map(rule => {
return [rule, getRuleURI(rule).url];

This comment has been minimized.

@ta2edchimp

ta2edchimp Oct 30, 2016

Collaborator

Why not rules.map(rule => [rule, getRuleURI(rule).url])?

rules = rules.map(rule => {
return [rule, getRuleURI(rule).url];
}).reduce((all, single) => {
return all.concat(single);

This comment has been minimized.

@ta2edchimp

ta2edchimp Oct 30, 2016

Collaborator

Why not […].reduce((all, single) => all.concat(single))?

flattened.push.apply(flattened, flattenRulesDiff(diff))
})
diffArray.forEach(diff => {
flattened.push.apply(flattened, flattenRulesDiff(diff));

This comment has been minimized.

@ta2edchimp

ta2edchimp Oct 30, 2016

Collaborator
    flattened.push(...flattenRulesDiff(diff));
// print out everything but the test target's output
if (!arguments[0].match(/diff/)) {
consoleLog.apply(null, arguments)
consoleLog.apply(null, arguments);

This comment has been minimized.

@ta2edchimp

ta2edchimp Oct 30, 2016

Collaborator
    sinon.stub(console, 'log', (...args) => {
      // print out everything but the test target's output
      if (!args[0].match(/diff/)) {
        consoleLog(...args)
      }
    })
return;
}
consoleLog.apply(null, arguments);
};

This comment has been minimized.

@ta2edchimp

ta2edchimp Oct 30, 2016

Collaborator

again

  console.log = (...args) => {
    if (args[0].match( /* [...] */ )) {
      return;
    }
    consoleLog(...args);

it('logs core rules', function() {
consoleLog.apply(null, arguments);
};

This comment has been minimized.

@ta2edchimp

ta2edchimp Oct 30, 2016

Collaborator

...args


while (_output.length) {
ui.div.apply(ui, _output.splice(0, _columns).map(cellMapper))
ui.div.apply(ui, _output.splice(0, _columns).map(cellMapper));

This comment has been minimized.

@ta2edchimp

ta2edchimp Oct 30, 2016

Collaborator
  ui.div(..._output.splice(0, _columns).map(cellMapper));
}
this.getCurrentRules = function () {
return getSortedRules(currentRules);
};

This comment has been minimized.

@ta2edchimp

ta2edchimp Oct 30, 2016

Collaborator

These could be shortened into

this.getCurrentRules = () => getSortedRules(currentRules);
@ta2edchimp

This comment has been minimized.

Copy link
Collaborator

ta2edchimp commented Oct 30, 2016

As you can see, I made another PR for your es-next branch in case you want to incorporate my proposed changes and save some time in doing so.

@sarbbottam

This comment has been minimized.

Copy link
Owner

sarbbottam commented Oct 31, 2016

On the overall, we should really keep an eye on the code's readability during "es-nextification".

Could not agree more. The could be readable first, as long as it is not affecting performance.

This PR is meant to be the base.

Once this is merged, we should start refactoring files/functions at a time.
We need to also revisit the cli options too. Maybe, for flags let there be only one way, like --no-error, --no-core only, no more n and nc. If so, this is going to be a breaking change.

@sarbbottam

This comment has been minimized.

Copy link
Owner

sarbbottam commented Oct 31, 2016

Please don't approve it via GitHub means, otherwise, the PR will get merged.

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Oct 31, 2016

@sarbbottam i'd strongly caution against automerging on PR approval; i don't even think semantic-release is a safe idea.

@sarbbottam

This comment has been minimized.

Copy link
Owner

sarbbottam commented Oct 31, 2016

@ljharb I guess I was wrong about auto-merging. It is not enabled. I had to merge #199 by myself.

I'd strongly caution against auto-merging on PR approval;
Agreed

semantic-release
Yea, it could be problematic sometimes, but has its benefits too. I am a bit biased but would be OK to let it go, if we all think so.

@ta2edchimp

This comment has been minimized.

Copy link
Collaborator

ta2edchimp commented Oct 31, 2016

So, is it safe to approve the requested changes? (I never worked with the review feature before ...)

Re: auto merge on review approval
Can we assure this is not activated?

Re: semantic release
Besides "spamming" with updates when lots of PRs would get merged within a short period of time, I see no problem in having automatic releases for this tool. But this is something we should discuss / evaluate in a separate issue.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 31, 2016

Codecov Report

Merging #200 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #200   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         229    216   -13     
=====================================
- Hits          229    216   -13
Impacted Files Coverage Δ
src/lib/flatten-rules-diff.js 100% <100%> (ø)
src/bin/find.js 100% <100%> (ø)
src/lib/stringify-rule-config.js 100% <100%> (ø)
src/bin/diff.js 100% <100%> (ø)
src/lib/rule-finder.js 100% <100%> (ø)
src/lib/array-diff.js 100% <100%> (ø)
src/lib/object-diff.js 100% <100%> (ø)
src/lib/sort-rules.js 100% <100%> (ø)
src/lib/cli-util.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebeec17...da11db6. Read the comment docs.

@sarbbottam sarbbottam force-pushed the es-next branch from 72b88ed to 956e70a Oct 31, 2016

@sarbbottam

This comment has been minimized.

Copy link
Owner

sarbbottam commented Oct 31, 2016

Moved auto-merge and semantic release discussion to #202

.travis.yml Outdated
- '0.10'
before_install:
- npm i -g npm@^3.0.0
- '6'

This comment has been minimized.

@mgol

mgol Dec 10, 2016

It would be best to test on all versions that ESLint supports, i.e. currently 4, 5, 6 & 7.

This comment has been minimized.

@ljharb

ljharb Dec 10, 2016

Collaborator

+1

@mgol

This comment has been minimized.

Copy link

mgol commented Dec 10, 2016

Is using Babel really necessary? If this package dropped support for Node <4 as ESLint has (note that all upstream support - including security fixes - for Node <4 ends at the end of 2016) then the code could be consumed directly.

.travis.yml Outdated
@@ -7,12 +7,8 @@ notifications:
email: false
node_js:
- stable

This comment has been minimized.

@ljharb

ljharb Dec 10, 2016

Collaborator

"stable" is legacy; this should be "node" if that's what you want. However, I'd encourage you to run tests on all of 7, 6, 5, and 4.

.travis.yml Outdated
- '0.10'
before_install:
- npm i -g npm@^3.0.0
- '6'

This comment has been minimized.

@ljharb

ljharb Dec 10, 2016

Collaborator

+1

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Dec 10, 2016

There's plenty of things that don't work properly in node 4. There's not really any benefit to avoiding the Babel step except to those debugging the plugin, and that's what source maps are for.

@randycoulman

This comment has been minimized.

Copy link
Contributor

randycoulman commented Feb 17, 2017

Is there anything I can do to help with this PR? I really want to resolve #172, but it needs #211, which is blocked by this PR as far as I can tell. Is there a way for me to jump in and help move this forward?

@sarbbottam

This comment has been minimized.

Copy link
Owner

sarbbottam commented Feb 17, 2017

@ta2edchimp could review the updates?

- '0.10'
before_install:
- npm i -g npm@^3.0.0
- 'node'

This comment has been minimized.

@ljharb

ljharb Feb 17, 2017

Collaborator

eslint still runs on node 4, so i'd think we should be testing on node 4 here also.

This comment has been minimized.

@sarbbottam

sarbbottam Feb 22, 2017

Owner

src code will not pass in node 4 as it does not support all the es-next syntax. Anyhow, for node 4 et al, the dis code need to be tested. I am not sure if that is the scope of this PR.

This comment has been minimized.

@ljharb

ljharb Feb 22, 2017

Collaborator

hm, aren't we using babel-node for the tests?

"update-contributors": "all-contributors generate",
"commit": "git-cz",
"validate": "npm-run-all --parallel lint cover --sequential check-coverage",
"check-coverage": "nyc check-coverage --statements 100 --branches 100 --functions 100 --lines 100",
"build": "babel src -d dist --presets es2015",

This comment has been minimized.

@ljharb

ljharb Feb 17, 2017

Collaborator

i'd recommend adding rimraf and mkdirp and doing rimraf dist && mkdirp dist && babel … here, otherwise if a src file is deleted, the corresponding dist file could end up remaining in the package.

This comment has been minimized.

@sarbbottam

sarbbottam Feb 22, 2017

Owner

added rimraf; mkdirp is not required as babel will generate the directory if not present.

This comment has been minimized.

@ljharb

ljharb Feb 22, 2017

Collaborator

sounds good!

"report-coverage": "cat ./coverage/lcov.info | node_modules/.bin/codecov",
"semantic-release": "semantic-release pre && npm publish && semantic-release post",
"travis-after-all": "travis-after-all && npm run report-coverage && npm run semantic-release"
},
"files": ["dist"],

This comment has been minimized.

@ljharb

ljharb Feb 17, 2017

Collaborator

Please do not ever use files - it's incredibly dangerous, because if you forget to add a file to this whitelist, the consequence is that the package is broken in a way that's very hard to detect.

It's fine to use .npmignore if you need to exclude files (where the worst thing that can happen if you forget is a slightly larger package) - but I also don't see any reason to exclude any of them. npm explore foo && npm install && npm test should always work.

This comment has been minimized.

@mgol

mgol Feb 17, 2017

For me it's .npmignore that's dangerous - you risk including folders created by various tools like IDEs. A blacklist approach may miss some of them, I feel it's safer to use the whitelist one.

This comment has been minimized.

@ljharb

ljharb Feb 17, 2017

Collaborator

The damage caused by that happening is approximately zero. The damage caused by omitting a file from the whitelist is production breakage.

This comment has been minimized.

@mgol

mgol Feb 17, 2017

I feel this is a tools deficiency problem. It should be easy to test packages as they are when published, not as in dev mode. Omitting something from the files array is analogous to erroneusly placing a dependency in devDependency. In the idiomatic in-place testing neither will be caught in CI.

This comment has been minimized.

@ljharb

ljharb Feb 18, 2017

Collaborator

Yet, nobody does it.

You're correct about the dependency issue, but that's why you can use the related rule in eslint-plugin-import to correct that.

It's certainly a fine argument that "files" could be made safe - but it's unarguably unsafe at the moment. Similarly, however, any tool that makes "files" safe could also ensure "npmignore" doesn't omit anything. I'd be very interested in evaluating a tool that covered both cases.

The only reasonable choice for now is the thing that's safe by default - which is npmignore. The "consequence" of "slightly higher filesize" is a complete non-problem, one that's not worth solving at the cost of safety.

This comment has been minimized.

@sarbbottam

sarbbottam Feb 22, 2017

Owner

removing files as of now.

@ta2edchimp

This comment has been minimized.

Copy link
Collaborator

ta2edchimp commented Feb 17, 2017

I just approved the changes that addressed my review.
Furthermore I fully endorse the changes requested by @ljharb — except the one regarding the package.json's file property, I'm a bit undecided here. But in the end, I'd rather end up with some unnecessary files that are included in the package, than having to deal with a broken package, due to missing files (I think we'll catch those extraneous files and folders easily within upcoming PRs).

@sarbbottam sarbbottam force-pushed the es-next branch from df76a4e to da11db6 Feb 22, 2017

@sarbbottam

This comment has been minimized.

Copy link
Owner

sarbbottam commented Feb 22, 2017

@ljharb I have addressed the review comment please review and if everything looks fine, please merge the PR.

@ljharb

ljharb approved these changes Feb 22, 2017

@ljharb ljharb merged commit dd2b370 into master Feb 22, 2017

3 checks passed

codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to ebeec17
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ljharb ljharb deleted the es-next branch Feb 22, 2017

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Feb 22, 2017

I am concerned that this may be a breaking change; prior to a release, it'd be a good idea to get tests passing in node 4.

@randycoulman

This comment has been minimized.

Copy link
Contributor

randycoulman commented Feb 22, 2017

Thanks for getting this done!

There's a suggestion that #211 will also result in a breaking change, so we might be talking about a major version release anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment