Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

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
@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
@@ -9,6 +10,21 @@ const Session = require('./session');

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

function asyncQuestion(prid) {
Copy link
Member

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

Choose a reason for hiding this comment

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

So waiting for #300

lib/landing_session.js Outdated Show resolved Hide resolved
@joyeecheung joyeecheung mentioned this pull request Nov 4, 2018
Merged
joyeecheung
joyeecheung reviewed Nov 4, 2018
View reviewed changes
lib/landing_session.js Outdated Show resolved Hide resolved
@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

@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

Can this be closed?

@mmarchini
Copy link
Contributor

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