Skip to content

[WIP] git-node: prompt and conditionally auto push#299

Closed
refack wants to merge 2 commits intonodejs:masterfrom
refack:auto-push
Closed

[WIP] git-node: prompt and conditionally auto push#299
refack wants to merge 2 commits intonodejs:masterfrom
refack:auto-push

Conversation

@refack
Copy link
Contributor

@refack refack commented Nov 4, 2018

Seeking feedback and testing guidance.

Refs: nodejs/node#23941 (comment)

@refack refack added enhancement Things that enhances functionality, provided by node-core-utils Work In Progress PR's that are in progress git-node discuss labels Nov 4, 2018
@refack refack self-assigned this Nov 4, 2018
@refack refack requested review from joyeecheung and targos November 4, 2018 16:56
@codecov
Copy link

codecov bot commented Nov 4, 2018 •
edited
Loading

Codecov Report

Merging #299 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #299   +/-   ##
=======================================
  Coverage   73.94%   73.94%           
=======================================
  Files          22       22           
  Lines        1401     1401           
=======================================
  Hits         1036     1036           
  Misses        365      365

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 beccfee...c161350. Read the comment docs.

@refack
Copy link
Contributor Author

refack commented Nov 4, 2018

I just saw cli.prompt, but anyway I needed a more complicated user input.

@refack refack mentioned this pull request Nov 4, 2018
Closed
3 tasks
joyeecheung
joyeecheung reviewed Nov 4, 2018
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

There are no tests for most of the git-node landing process, I am afraid..

lib/landing_session.js

const isWindows = process.platform === 'win32';

function asyncQuestion(prid) {
Copy link
Member

@joyeecheung joyeecheung Nov 4, 2018

Choose a reason for hiding this comment

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

I can refactor this a bit into a cli.promptForInput that returns the answer for later use

Copy link
Contributor Author

@refack refack Nov 4, 2018

Choose a reason for hiding this comment

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

So waiting for #300

@joyeecheung joyeecheung mentioned this pull request Nov 4, 2018
Merged
joyeecheung
joyeecheung reviewed Nov 4, 2018
View reviewed changes
@targos
Copy link
Member

targos commented Nov 18, 2018 •
edited
Loading

@refack inquirer is now part of the dependencies (in case that helps for this PR)

@joyeecheung
Copy link
Member

joyeecheung commented Nov 19, 2018 •
edited
Loading

@targos Maybe open a separate issue about refactoring cli.prompt on top of inquirer ? (like the cli.startSpinner and cli.stopSpinner are on top of ora, but we get to dependency-inject the cli in tests)

@targos
Copy link
Member

targos commented Nov 19, 2018

@joyeecheung sure. I thought this PR was blocked on prompt-related things. My bad.
Is it ready for review then?

@joyeecheung
Copy link
Member

joyeecheung commented Nov 19, 2018

@targos I thought this was blocked on the TODO in the comment?

@refack refack mentioned this pull request Nov 24, 2018
Merged
@codebytere
Copy link
Member

codebytere commented Jul 16, 2020

Can this be closed?

@mmarchini
Copy link
Contributor

mmarchini commented Aug 12, 2020

Closing, if anyone wants to reopen feel free to do so.

@mmarchini mmarchini closed this Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joyeecheung joyeecheung joyeecheung left review comments

@targos targos Awaiting requested review from targos

Assignees

@refack refack

Labels

discuss enhancement Things that enhances functionality, provided by node-core-utils git-node Work In Progress PR's that are in progress

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@refack @targos @joyeecheung @codebytere @mmarchini