nodejs / node-core-utils

CLI tools for Node.js Core collaborators
https://nodejs.github.io/node-core-utils/
MIT License
234 stars 106 forks source link

Feature/url parsing #30

Closed Tiriel closed 6 years ago

Tiriel commented 6 years ago

Please feel free to suggest changes!

From #22 :

From #25 :

I have tried to test it manually, seems to be working as expected!

codecov-io commented 6 years ago

Codecov Report

Merging #30 into master will increase coverage by 0.13%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #30      +/-   ##
==========================================
+ Coverage   96.41%   96.54%   +0.13%     
==========================================
  Files          12       13       +1     
  Lines         418      434      +16     
==========================================
+ Hits          403      419      +16     
  Misses         15       15
Impacted Files Coverage Δ
lib/args.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 54b1004...8623a70. Read the comment docs.

Tiriel commented 6 years ago

Fixed lint! Sorry, don't know why my IDE linted it that way...

joyeecheung commented 6 years ago

I have given it a try and looks like it's not working as intended...

screen shot 2017-10-31 at 10 44 48 pm screen shot 2017-10-31 at 10 43 50 pm
joyeecheung commented 6 years ago

Also..we don't have a test for CLI, that's why this passes travis. I think we should get some tests in before this lands.

Tiriel commented 6 years ago

True that.

<edited, spoke too quickly> I'm going to see what I can do for testing!

Tiriel commented 6 years ago

Okay. Lint ok, tests ok, build ok... At last.

Sorry for the messy work, not used to this sort of thing yet, learning. Good for review I guess!

joyeecheung commented 6 years ago

@Tiriel I have merged the refactoring PR, there is a conflict now, can you rebase?

joyeecheung commented 6 years ago

BTW this is how you can rebase

$ git checkout feature/url-parsing
$ git fetch upstream # assuming upstream points to this repo
$ git rebase upstream/master
# fix conflicts
$ git push --force origin feature/url-parsing  # assuming origin points to your fork
Tiriel commented 6 years ago

No problem, doing it right now! And thanks for the tip.

Tiriel commented 6 years ago

Done!

Linted, tests passing on my local, command working as far as I can tell!

joyeecheung commented 6 years ago

Awesome work! Thanks!