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

New rule: fix missing git clone when given a URL #1302

Merged
merged 14 commits into from
Jul 3, 2022
Merged

New rule: fix missing git clone when given a URL #1302

merged 14 commits into from
Jul 3, 2022

Conversation

MaddyGuthridge
Copy link
Contributor

Often when I'm trying to git clone a project, I'll paste the URL into my terminal expecting it to also contain the git clone part. This rule should allow thefuck to detect the missing git clone and suggest it when someone pastes an SSH or HTTP/HTTP url that ends in .git.

A rule for correcting for double git clone git clone [repo] already exists. This rule addresses the opposite problem.

image

I've also added some simple tests for it, which appear to pass.

This is my first PR with any meaningful change, so I'd appreciate some feedback on it. It still needs a bit of cleaning up, but I think it should be good to merge in once I remove a couple of random things

@MaddyGuthridge MaddyGuthridge changed the title Miguel/git clone missing New rule: fix missing git clone when given a URL May 24, 2022
@MaddyGuthridge
Copy link
Contributor Author

@djh82 would appreciate a re-review. I think I've fixed up all the issues

Copy link
Collaborator

@scorphus scorphus left a comment

Choose a reason for hiding this comment

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

This rule should be output-independent. Why bother matching the output?

However, it reads quite specific to me, I don't think many users would benefit from it. A URL might be related to so many things, not only git clone.

The Fuck allows the user to have user-defined rules at their disposal: Creating your own rules.

Not every single rule has to be part of The Fuck.

@MaddyGuthridge
Copy link
Contributor Author

MaddyGuthridge commented Jun 6, 2022

@scorphus thanks for your feedback! I believe that this rule would be worthwhile as I encountered it enough times to choose to spend my time on creating a rule for it. I tutor a computer science class where we teach git and have observed that I'm far from the only person to make this mistake. Since there's a rule for handling git clone git clone, which is useful when I type git clone then paste the URL, only for it to include its own git clone (Bitbucket does this), I'd say that this particular rule is reasonable enough, since it handles the opposite case.

RE: your feedback on output matching, it does seem to be a little pointless. If you're interested in merging this PR at some point then I'll absolutely get rid of it.

Perhaps it could search for 'git' within the URL as an additional measure to avoid false positives? This would still work for all most of the popular git servers I'm aware of, at least using SSH. Would that change make the addition be worthwhile? Can you think of any other methods I could use to reduce the number of false positives?

@scorphus
Copy link
Collaborator

scorphus commented Jun 6, 2022

Thanks for sharing your testimonial. Let's get this merged.

Regarding matching the output, I thought about disregarding output altogether and caring only about output.script or output.script_parts.

Asserting git in the URL would blacklist bitbucket repos.

Copy link
Collaborator

@scorphus scorphus left a comment

Choose a reason for hiding this comment

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

Thanks for contributing, please consider my suggestions below.

Also, a new line in README is required.

thefuck/rules/git_clone_missing.py Outdated Show resolved Hide resolved
thefuck/rules/git_clone_missing.py Outdated Show resolved Hide resolved
thefuck/rules/git_clone_missing.py Outdated Show resolved Hide resolved
tests/rules/test_git_clone_missing.py Outdated Show resolved Hide resolved
tests/rules/test_git_clone_missing.py Outdated Show resolved Hide resolved
tests/rules/test_git_clone_missing.py Outdated Show resolved Hide resolved
@MaddyGuthridge
Copy link
Contributor Author

MaddyGuthridge commented Jun 7, 2022

Thanks again for the feedback! I fixed up things as per your suggestion, and also removed the code that checked the output.

Double checking, since I'm not entirely sure, should it check the URL for the presence of git somewhere? The only case I can think of that wouldn't work would be cloning using HTTPS on Bitbucket where .git has been manually removed from the end of their copied URL. Their SSH username is git, and their copy clone button copies the git clone part anyway. It would be helpful for preventing false positives such as YouTube videos (I apologise in advance).

Copy link
Collaborator

@scorphus scorphus left a comment

Choose a reason for hiding this comment

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

My previous review was partially written. I would conclude the thought that I initiated in the message before, but ended up forgetting about it. Apologies.

I'd complement by saying that checking the command output is actually desirable and I should've given it a bit more thought when commenting earlier. Doing so speeds things up for cases that don't match by a factor of 4, at least.

But I think it should be simplified. Please consider my suggestions below.

Also, please, could you bring back the tests cases that include the output? Thanks!

thefuck/rules/git_clone_missing.py Outdated Show resolved Hide resolved
thefuck/rules/git_clone_missing.py Outdated Show resolved Hide resolved
tests/rules/test_git_clone_missing.py Outdated Show resolved Hide resolved
tests/rules/test_git_clone_missing.py Outdated Show resolved Hide resolved
tests/rules/test_git_clone_missing.py Outdated Show resolved Hide resolved
@MaddyGuthridge
Copy link
Contributor Author

Pretty sure I've got everything fixed up now! Let me know if there's anything else you'd like me to do!

@MaddyGuthridge
Copy link
Contributor Author

Just bumping this one again @scorphus
Is there anything else you want me to do on it?

Copy link
Collaborator

@scorphus scorphus left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, @MiguelGuthridge.

Pretty sure I've got everything fixed up now! Let me know if there's anything else you'd like me to do!

You nailed it. 👍

My bad: I've just realized that, for commands like git@github.com:nvbn/thefuck.git and https://github.com/nvbn/thefuck.git, Macos's sh errs with /bin/sh: https://github.com/nvbn/thefuck.git: No such file or directory\n.

I'll got ahead and make the necessary change and push it on top of yours. Let's get this merged.

@scorphus
Copy link
Collaborator

scorphus commented Jul 2, 2022

@MiguelGuthridge, please let me know your thoughts about the last change I submitted.

@MaddyGuthridge
Copy link
Contributor Author

Looks pretty great to me!

Copy link
Collaborator

@scorphus scorphus 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, @MiguelGuthridge! :shipit:

@scorphus scorphus merged commit f9768cf into nvbn:master Jul 3, 2022
@scorphus
Copy link
Collaborator

scorphus commented Jul 3, 2022

@MiguelGuthridge, thanks for the complete patch! Much appreciated! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants