Skip to content

bin: custom git commands - #106

Merged
joyeecheung merged 3 commits into
nodejs:masterfrom
joyeecheung:git-cmd
Jan 17, 2018
Merged

bin: custom git commands#106
joyeecheung merged 3 commits into
nodejs:masterfrom
joyeecheung:git-cmd

Conversation

@joyeecheung

Copy link
Copy Markdown
Member

This was #98 , opening a new PR because the repo has been transferred so I need to update the branch of my fork instead.

@joyeecheungjoyeecheung changed the title [WIP] bin: custom git commands #98[WIP] bin: custom git commandsNov 11, 2017
@codecov

codecovBot commented Nov 11, 2017

Copy link
Copy Markdown

Codecov Report

Merging #106 into master will decrease coverage by 2.19%.
The diff coverage is 7.14%.

Impacted file tree graph

@@ Coverage Diff @@## master #106 +/- ##
=========================================
- Coverage 98.05% 95.86% -2.2% 
=========================================
Files 15 15 Lines 567 581 +14 =========================================
+ Hits 556 557 +1 - Misses 11 24 +13
Impacted FilesCoverage Δ
lib/cli.js73.77% <7.14%> (-19.85%)⬇️

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 acc9e3a...ad7a7a7. Read the comment docs.

@joyeecheungjoyeecheung added the Work In Progress PR's that are in progress label Nov 12, 2017
Comment threadbin/git/git-node-apply Outdated
const paths = require('./../../lib/paths');
const { run, runAsync } = require('../../lib/run');
const fs = require('fs');
const CLI = require('../../lib/CLI');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should be ../../lib/cli.

@joyeecheung

Copy link
Copy Markdown
MemberAuthor

Note that this is still WIP, I would still need to find some time to port the rest of the bash scripts to Node.js scripts. Although I have been using this fork locally to land stuff anyways.

@priyank-p

Copy link
Copy Markdown
Contributor

I totally did not mean to hit approve meant to comment...

@priyank-p

Copy link
Copy Markdown
Contributor

@joyeecheung tried to port bash scripts to node-script and this is currently what i have.
https://gist.github.com/cPhost/dec7ce68a09907791ce99147feb618e9

(I this help great! 👍 )

@joyeecheung

joyeecheung commented Nov 17, 2017

Copy link
Copy Markdown
MemberAuthor

@cPhost Thanks, but the porting is not really what's blocking me right now...The more I use it, the more I prefer the git node land --continue concept (I don't really like to remember all those sub commands, even if they are devised by myself. Also it's kinda annoying that I have to paste the PR id multiple times), so the commands kinda need a rework to mimic a git rebase session.

@priyank-p

Copy link
Copy Markdown
Contributor

Also after landing this before publishing to npm, we need to ensure the Windows compatibility.

@priyank-p

Copy link
Copy Markdown
Contributor

After thinking about sub commands i think we should have then and and implement single land script that does all the things at once with the subcommands.

@joyeecheung
joyeecheungforce-pushed the git-cmd branch 2 times, most recently from 03da45f to 6c578efCompareNovember 20, 2017 13:37
@joyeecheung

Copy link
Copy Markdown
MemberAuthor

Recording of working prototype: https://asciinema.org/a/6i3gFDSAr7L6fYPSEaMrW0asO

@joyeecheung
joyeecheungforce-pushed the git-cmd branch 3 times, most recently from 9ed39bd to f99e6f4CompareJanuary 11, 2018 18:46
@joyeecheungjoyeecheung changed the title [WIP] bin: custom git commandsbin: custom git commandsJan 15, 2018
@joyeecheung

joyeecheung commented Jan 15, 2018

Copy link
Copy Markdown
MemberAuthor

I've been using it for about two months now and it seems to be working pretty well. There are still a few things that need to be addressed:

  1. core-validate-commit must be installed
  2. core-validate-commit still complains about commit titles > 50 char so git node land --final fails from time to time. (should be fixed by warn for 50 < title <= 72 core-validate-commit#18)
  3. Needs some docs
  4. Needs some tests (would be a bit trickier to set up since it involves a lot of git operations)

I think when I finished 3 this should be ready to land on master.

@joyeecheung
joyeecheungforce-pushed the git-cmd branch 2 times, most recently from a4618aa to 4752deeCompareJanuary 17, 2018 01:36
@joyeecheung

Copy link
Copy Markdown
MemberAuthor

Docs done. Demos:

Landing multiple commits: https://asciinema.org/a/148627
Landing one commit: https://asciinema.org/a/157445

This should be ready to merge.

@priyank-p

Copy link
Copy Markdown
Contributor

This Looks good to me :)

@joyeecheung

Copy link
Copy Markdown
MemberAuthor

I am going to merge this and release a new version. Let's see how it works in the wild.

@joyeecheung
joyeecheung merged commit 8a0e114 into nodejs:masterJan 17, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@joyeecheung@priyank-p@Tiriel