-
Notifications
You must be signed in to change notification settings - Fork 14
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
Added support to delete codespaces just like repositories #37
Conversation
made a minor change in get config function to respect linux paths as well ref adrianmg#36
Thanks for contributing, @AnishDe12020! Sharing some thoughts:
|
2 & 3: I can use a library like commander.js or similar (easier arg parsing, automated help commands). Done, the help command is autogenerated and the code is much cleaner now (55803a0) |
Todo before releasing:
|
@AnishDe12020 I wonder if we can remove the I like we are splitting commands into different files and how simple |
only 1 argument is to be parsed, handled by a switch statement now the help message is hardcoded
…le tree also remove a repo i mistakenly had cloned into this directory
I have accomplished the same with a switch statement now, the help message is hardcoded though. I have also updated the tests and the README |
@AnishDe12020 I'll try to follow up on pending conversations during this weekend. Thanks for your time! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR @AnishDe12020. IMO this is an excellent improvement to the tool. I've just added a couple of comments but LGTM
case 'help': | ||
UI.printHelp(); | ||
break; | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is right in order to avoid breaking changes in how the tool is executed. However, I think this could be improved. For example, if anyone executes something like this:
ghpew any-unknown-command
The script will execute the reposCommand
. Maybe we could change this behavior to print the help:
default:
if (!command) await reposCommand();
else UI.printHelp(); // unknown command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, it should be tested too...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, I have added the check (5acd3ec)
For the label part, I have made this utility function -
const labels = {
repos: { singular: 'repository', plural: 'respositories' },
codespaces: { singular: 'codespace', plural: 'codespaces' },
};
const getLabel = (type, count) => {
const { singular, plural } = labels[type];
return count > 1 ? plural : singular;
};
(51ecf43)
The code is much more cleaner now, thanks for the feedback :)
@IvanGuardado @AnishDe12020, excellent discussion! I was busy shipping https://adrianmato.art last week, but I'd be catching up with this soon 🙇 |
@adrianmg any update on this pr? |
@AnishDe12020 looking into it |
@AnishDe12020 there is a new bug when repos are listed but the command is cancelled: I'm not sure we want to show the help command in such cases? |
forgot to break out of the switch block there |
only happened in the case of no arguments
I thought so, but then I realized there's no "breaking" changes as they can't currently automate it without user intervention to provide a choice of repos to be deleted 🤔 |
ref #36
I have added support for deleting codespaces with this CLI tool. Main changes -
codespace
scope during authconfig.js
to respect config path for linux machines (.config/com.adrianmg.github-pewpew
)Use the
codespaces
argument withghpew
for the same.Here is a screenshot -