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.

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

chore(prompt): various enhancements and refactors #576

Merged
merged 7 commits into from
Aug 19, 2022

Conversation

kanadgupta
Copy link
Member

@kanadgupta kanadgupta commented Aug 19, 2022

🚥 Fix RM-3621 (and a bunch of other stuff)

🧰 Changes

  • Refactored the login command to use overrides and added a few tests
  • Refactored our onState usage to use onCancel (see 16f6117 for more reasoning)
  • Removed the apparently unnecessary enableTerminalCursor function. Not sure if it was working before but I've tried a bunch of things and can't get my cursor to disappear. I'm cool with keeping it in there just to be safe too, let me know what you think 🤷🏽
  • Refactored our authenticated commands so we dynamically detect the key argument via the args array, rather than having a flag that we have to manually enable
  • Added top-level CI detection so our prompts error out in CI environments

🧬 QA & Testing

Tests appear to pass, but it might be good to check out this branch, run the build, and try messing around.

  • Does the terminal cursor remain intact no matter what commands you try?
  • Are you still able to safely exit out of prompts using CTRL+C or ESC?
  • Does CI detection work for you properly?1

Footnotes

  1. You can test this by prefixing any command with a CI env variable. I've been running GITLAB_CI=true bin/rdme login

I was looking through the docs and saw that onState was injected into every question in the question array and would run on every keystroke, and saw that onCancel existed and would look a lot cleaner in our prompt wrapper.
I'm not sure this is needed? I'm not able to get rid of the cursor anymore 🤔
@kanadgupta kanadgupta added enhancement New feature or request refactor Issues about tackling technical debt command:other Issues pertaining to miscellaneous commands (e.g. `open`, `logout`) or requests for new commands labels Aug 19, 2022
@kanadgupta kanadgupta requested a review from erunion August 19, 2022 22:49
@kanadgupta kanadgupta marked this pull request as ready for review August 19, 2022 22:49
@@ -44,9 +45,9 @@ export default class LoginCommand extends Command {
async run(opts: CommandOptions<Options>) {
super.run(opts);

let { project } = opts;
prompts.override(opts);
Copy link
Member Author

Choose a reason for hiding this comment

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

I got irrationally excited about this one lol

Copy link
Member

Choose a reason for hiding this comment

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

Should we use this in every command with prompts that can be fed arguments?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeahhh I read about override and login was the first thing I thought of... I don't want to look at the versions commands for a long time lmao but I looked into doing this for the prepareOas handler and we have enough complexity to our spec searching that I can't think of a useful refactor approach

Copy link
Member

Choose a reason for hiding this comment

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

yeah i'd rather remove the versions commands before even thinking about doing anything else to them

Copy link
Member

@erunion erunion left a comment

Choose a reason for hiding this comment

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

can't replicate the missing cursor problem with this but that auth refactor is 💯

@@ -103,5 +103,21 @@ describe('rdme login', () => {
mock.done();
});

it.todo('should error if trying to access a project that is not yours');
Copy link
Member

Choose a reason for hiding this comment

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

😬

@@ -44,9 +45,9 @@ export default class LoginCommand extends Command {
async run(opts: CommandOptions<Options>) {
super.run(opts);

let { project } = opts;
prompts.override(opts);
Copy link
Member

Choose a reason for hiding this comment

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

Should we use this in every command with prompts that can be fed arguments?

@@ -62,7 +63,7 @@ export default class LoginCommand extends Command {
message: 'What is your password?',
},
{
type: opts.project ? null : 'text',
Copy link
Member

Choose a reason for hiding this comment

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

prompts rules, good lord.

@kanadgupta kanadgupta merged commit 0b2750d into main Aug 19, 2022
@kanadgupta kanadgupta deleted the various-prompts-fixes branch August 19, 2022 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
command:other Issues pertaining to miscellaneous commands (e.g. `open`, `logout`) or requests for new commands enhancement New feature or request refactor Issues about tackling technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants