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

Added support to delete codespaces just like repositories #37

Merged
merged 47 commits into from
Oct 2, 2022

Conversation

AnishDe12020
Copy link
Contributor

@AnishDe12020 AnishDe12020 commented Jul 26, 2022

ref #36

I have added support for deleting codespaces with this CLI tool. Main changes -

  • Request additional codespace scope during auth
  • More functions (mostly copies of the ones for repos) but for codespaces
  • Small change in config.js to respect config path for linux machines (.config/com.adrianmg.github-pewpew)

Use the codespaces argument with ghpew for the same.

Here is a screenshot -
image

made a minor change in get config function to respect linux paths as well
ref adrianmg#36
src/ui.js Outdated Show resolved Hide resolved
@adrianmg
Copy link
Owner

adrianmg commented Jul 29, 2022

Thanks for contributing, @AnishDe12020!

Sharing some thoughts:

  1. Have you tried if existing users will be able to re-auth once a new scope is detected?
  2. Have you thought about adding help, -h, and --help to describe different options to run the command? This seems necessary to promote the discoverability of this and upcoming features (such as gists?)
  3. As we add more commands, I wonder if we should eventually abstract the UI a bit more, as it can get very verbose with mane ifs and elses. Thoughts?

@AnishDe12020
Copy link
Contributor Author

AnishDe12020 commented Jul 30, 2022

Thanks for contributing, @AnishDe12020!

Sharing some thoughts:

  1. Have you tried if existing users will be able to re-auth once a new scope is detected?
  2. Have you thought about adding help, -h, and --help to describe different options to run the command? This seems necessary to promote the discoverability of this and upcoming features (such as gists?)
  3. As we add more commands, I wonder if we should eventually abstract the UI a bit more, as it can get very verbose with mane ifs and elses. Thoughts?
  1. I will add a check in the codespace command and if i am not able to retrieve codespaces, I will delete the config file and re run main (essentially a re auth process). I will get this done today Done (21b41b8)

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)

@adrianmg
Copy link
Owner

adrianmg commented Jul 31, 2022

Todo before releasing:

  • test.js should include codespaces
  • Update README with new instructions

@adrianmg
Copy link
Owner

adrianmg commented Jul 31, 2022

@AnishDe12020 I wonder if we can remove the commander.js dependency and achieve similar functionality with a couple of helper functions 🤔

I like we are splitting commands into different files and how simple index.js is now, but one of the goals of the project was to reduce third-party code and dependencies as much as I could.

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
@AnishDe12020
Copy link
Contributor Author

@AnishDe12020 I wonder if we can remove the commander.js dependency and achieve similar functionality with a couple of helper functions thinking

I like we are splitting commands into different files and how simple index.js is now, but one of the goals of the project was to reduce third-party code and dependencies as much as I could.

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

@adrianmg
Copy link
Owner

adrianmg commented Aug 2, 2022

@AnishDe12020 I'll try to follow up on pending conversations during this weekend. Thanks for your time!

README.md Outdated Show resolved Hide resolved
Copy link

@IvanGuardado IvanGuardado left a 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

src/ui.js Outdated Show resolved Hide resolved
case 'help':
UI.printHelp();
break;
default:

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

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...

Copy link
Contributor Author

@AnishDe12020 AnishDe12020 Aug 4, 2022

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)

image

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 :)

@adrianmg
Copy link
Owner

adrianmg commented Aug 8, 2022

@IvanGuardado @AnishDe12020, excellent discussion!

I was busy shipping https://adrianmato.art last week, but I'd be catching up with this soon 🙇

@AnishDe12020
Copy link
Contributor Author

@adrianmg any update on this pr?

@adrianmg
Copy link
Owner

adrianmg commented Sep 6, 2022

@AnishDe12020 looking into it

@adrianmg
Copy link
Owner

adrianmg commented Sep 6, 2022

@AnishDe12020 there is a new bug when repos are listed but the command is cancelled:

image

I'm not sure we want to show the help command in such cases?

@AnishDe12020
Copy link
Contributor Author

AnishDe12020 commented Sep 6, 2022

looking into it fixed in adrianmg/github-pewpew@5f582ae (#37)

forgot to break out of the switch block there

@adrianmg
Copy link
Owner

As I see it being a breaking change for existing users

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 🤔

@adrianmg adrianmg linked an issue Oct 1, 2022 that may be closed by this pull request
@adrianmg adrianmg linked an issue Oct 1, 2022 that may be closed by this pull request
@adrianmg adrianmg merged commit 7cad00e into adrianmg:main Oct 2, 2022
@AnishDe12020 AnishDe12020 deleted the feat/delete-codespaces branch October 2, 2022 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to delete more stuff like codespaces and gists
4 participants