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

#215 Weird weekend detection fixed #220

Closed MM422 closed 6 years ago

MM422 commented 6 years ago

This PR is for fixing nodejs/node-core-utils Issues #215 Weird weekend detection.

@cPhost Please review it. If you want I can add more test cases on some of the potentially similar places in this project repo.

MM422 commented 6 years ago

@cPhost Oops, I think the build fails because of issue #212 and I basically test my change by commenting out components/git/metadata.js line 91-93 and everything is able to work.

Besides, I can also look into #212 tonight / tomorrow. Right now my commit should fix #215.

priyank-p commented 6 years ago

@GithubGoldMiner, you need to update the test at https://github.com/nodejs/node-core-utils/blob/master/test/unit/pr_checker.test.js#L163-L166 for the ci to pass. Thanks!

MM422 commented 6 years ago

@cPhost Thank you for pointing that out. This is actually my first time fixing bug for Node so if there is anything wrong, please help me out XD.

codecov[bot] commented 6 years ago

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #220   +/-   ##
=======================================
  Coverage   93.59%   93.59%           
=======================================
  Files          17       17           
  Lines         656      656           
=======================================
  Hits          614      614           
  Misses         42       42
Impacted Files Coverage Δ
lib/pr_checker.js 98.4% <100%> (ø) :arrow_up:

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 110da6d...78796e6. Read the comment docs.

MM422 commented 6 years ago

@cPhost I am sorry. I tried to implement the "Squashing" task in Projects and pushed my codes by mistake...

tniessen commented 6 years ago

@GithubGoldMiner You can simply remove commits which you didn't want to push by resetting your branch to the last commit you would like to publish and force pushing:

git checkout <your branch>
git reset --hard <desired commit>
git push --force origin <your branch>

"Your branch" would be master I guess (note that it is unconventional to create PRs from your master branch, you might want to avoid that in the future), and I assume the "desired commit" is the second, so 80758c1a.

MM422 commented 6 years ago

@joyeecheung The testcase has been updated. Thank you!

MM422 commented 6 years ago

@tniessen Thank you! I will keep that in my mind. :)

MM422 commented 6 years ago

Sorry this is actually the first time for me to contribute codes so I apologize if this question is silly. @joyeecheung @cPhost should I kill the commits as tniessen said before so that these my codes could be merged since I can't find a way to merge it to this repo? Or it's just because I don't have access to write and I should wait till somebody who has access merges my codes?

joyeecheung commented 6 years ago

@GithubGoldMiner I think you could not merge since you don't have commit access to this repo. It seems strange that travis didn't pick up the last commit, I've tested it locally and the test is green so I am going to merge this. Thanks!

MM422 commented 6 years ago

@joyeecheung Thank you very much!