cli
cli copied to clipboard
improve open cmd
This pull request aims to improve the open command.
I start by solving #460, while diving into the open command, I found that it could be improved in a few various ways.
I completely failed to keep up with my Exercism notifications, and I'm inexcusably late to the party. To add to this, my laptop crashed and is now in for repair, and as of today I have a loaner that I've not quite gotten set up to be usable. So. My apologies (first) and (second)... my goal is to get through all the open PRs in the CLI track over the course of the next week. So sorry to leave you hanging!
I completely failed to keep up with my Exercism notifications, and I'm inexcusably late to the party. To add to this, my laptop crashed and is now in for repair, and as of today I have a loaner that I've not quite gotten set up to be usable. So. My apologies (first) and (second)... my goal is to get through all the open PRs in the CLI track over the course of the next week. So sorry to leave you hanging!
Don't worry about it too much! Things happen and I'm pretty sure most people here are guilty of ignoring notifications for a long time. Completely understandable
Hey @Smarticles101 -- overall it looks like what you have here is much better.
In order to help me review this properly, would you summarize exactly what changed, and how this will change the usage of the command?
@kytrinyx The old open command worked in a way similar to this:
exercism open ./path/to/exercise/folder
The new command, rather than using an exercise path, follows a style similar to the other commands. Now instead of passing an exercise directory, you pass a slug.
exercism open --exercise slug
I also added support for --team
, --track
, and --remote
from #460 .
Thank you, that's really helpful!
In order to not break people's work flow we'll need to support the original usage as well, though... at least until we do a major version bump.
@kytrinyx That sounds good, I can add that back in when I have free time tomorrow
@kytrinyx original usage is supported now :)
No idea why the build is failing, I haven't changed anything that should make it fail, and I've rebased against master and compiled it locally and it seems to work?
original usage is supported now :)
w000p!!!!
No idea why the build is failing
I'll take a look.
I made a mess of my git tree today and ended up having to cherry-pick my commits out.
I updated it to use ExerciseSlug
now so the build passes. I'll see when I have time to look into your other comments. I should get some time early next week
@Smarticles101 just a quick note to say that I think the next step here is to rebase onto master before I do another review. If I'm wrong about that please let me know :-)
@Smarticles101 Is this ready for another round of review?
I know it's been nearly two years, but I thought I would get this finished up. I've addressed the last changes requested. If everything looks good I will write some tests.
@ekingery if you'd like to take a look at this and let me know if you see anything that's off, I'd appreciate it :)
@ekingery if you'd like to take a look at this and let me know if you see anything that's off, I'd appreciate it :)
Thanks @Smarticles101! We're fully focused on getting configlet updated for V3 right now, but this change looks like something we should be able to get merged as soon as we can bring some focus back to the CLI and get a new version out. It looks like the build is broken at the moment, any ideas on that?
@ekingery yes, like in #957, it fails on older versions of go presumably due to outdated modules. It builds correctly on the latest version
This is quite an old PR and has some merge conflicts. @Smarticles101 do you still want to work on this?
@ErikSchierboom thanks for checking on this. I've been out of the loop of exercism for a few years and recently started my first full-time job out of college so I don't have much time to pick this up again.
I'll go ahead and close this. If anyone comes across this in the future, feel free to keep working on it :)