-
Notifications
You must be signed in to change notification settings - Fork 32
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
Prepare 2.1.0 #275
Prepare 2.1.0 #275
Conversation
@@ -1,7 +1,5 @@ | |||
node_modules/ | |||
.nyc_output/ | |||
coverage/ | |||
src/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason any of these changes so far need a v3, as opposed to a v2.1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, I just thought about doing these preparations in order to get #270 merged.
I have no opinion against correcting v2 prior to that (that is, to include src and test again, for a v2.1 too). So, we could make a clean 2.1.0. Shall we do it? I'd rename this PR then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like it if we could :-)
🤔 code coverage fails on Node 6.x, the whole linting fails on Node 8.x ... and I thought this could be a fast one 😅 And btw. I think we should include Node 7.x on Travis, too, don't we? |
I should add, the linting only fails on Node 8 on Travis ... runs fine locally 😠 |
The issue regarding the code coverage has been resolved, background can be found here. Iirc we introduced this "ignoring" because nyc did not consider the line covered, although... it is... idk. As to why the the dependency of |
node 7 really doesn't matter anymore; the instant node 8 came out, v7 was EOL. We definitely need to work on 4, 6, and 8. |
@@ -7,6 +7,7 @@ notifications: | |||
email: false | |||
node_js: | |||
- 'node' | |||
- '7' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should definitely include 4, since it's still LTS. 7 doesn't really matter, but should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As ESLint still supports Node 4, I'm with you on that.
Do you have any idea about Travis failing when using v8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're only running it on node 4+, I'm not really sure (npm 4.6+ breaks on < v1, and v5+ breaks on < v4).
It might be a bug with xo?
Codecov Report
@@ Coverage Diff @@
## master #275 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 9 9
Lines 209 179 -30
=====================================
- Hits 209 179 -30
Continue to review full report at Codecov.
|
Some notes about what I did, as I did it:
Considering, we omitted running the build on Node v4 on Travis for quite some time, I'd prefer to make that a separate task and to leave it out for now (we would have to rewrite a lot of the tests in order to run appropriately on Node v4). |
Seems legit. Caching node_modules across node versions is risky; we can def add node 4 later; linting doesn't need to run on multiple node versions anyway. |
package.json
Outdated
@@ -32,32 +32,32 @@ | |||
"dependencies": { | |||
"cliui": "^3.2.0", | |||
"eslint-rule-documentation": "^1.0.0", | |||
"path-is-absolute": "1.0.0", | |||
"path-is-absolute": "1.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason this doesn't have a ^?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. This might have been introduced because of one's personal preference using --save-exact
or sth.
Eventually, this should be uniform. I suggest we change all the dependencies (see devDeps) to also use the ^
soon.
Thanks for the approval. How should we proceed from here? In order to keep things together, I'd suggest to use this PR to get everything together for a release (bump the version) and, if I manage to do so in time, make the necessary changes to get Travis succeed on Node v4, and eventually release 2.1. Do you agree? |
Sounds like a plan. More frequent releases are OK too :-) |
Proposed Changelog: 2.1.0 (2017-06-12)Features
Miscellaneous
Will merge and release, if you're ok with it. |
Preparations for an upcoming
3.0.0cleaned up 2.1.0 release:src
andtest
directories in a released package again