Fix Issue #959: breaks after composer require with single package, revamp composer rules

[chestnutcase]

Hello! I'm a day late for Hacktoberfest but I hope you find this PR useful.

This pull request aims to fix #959 and add better integration with the composer tool. It turns out the issue of the crash was not because "there was only one suggestion" as suggested in the Issue, but rather that the matcher erroneously matches package not found errors instead of just command not found errors. Since the output of the two varies significantly, I have created two separate rules (and tests) for this functionality.

In addition, the old tests used outdated test cases from more than 2 years ago (the output has changed in newer versions). I have tested it on newer versions of composer, notably the latest version of composer (1.7.0).

Behaviour of issue #959 before fix:

After fix:

The original composer-not-command was also not comprehensive enough: when multiple suggestions are offered, only one is passed to thefuck. This is fixed:

Before

After

(the stray composer require potato appears to be a framework issue outside my domain, as my unit tests that tests the suggested commands pass)

With the addition of the composer-not-package rule, thefuck is now alot more accommodating of various user mistakes:

Attempting to require an erronous package name with multiple suggestions lets thefuck cycle through all suggestions

Before

After

thefuck now also respects version constraints, as reflected in the generated suggestions:

Single suggestion + version constraint

Multiple suggestions + version constraint

Finally, thefuck is able to do consecutive fucks with composer, such as in the case where the user attempts to require several erroneous package names at once:

Tests & Documentation

I have revamped the tests with updated output from recent versions of composer. It looks messy but it contains all the different possible scenarios that can result from botched user input. I have tried my best with making the code self documenting with sensible variable names and comments.

flake8 does not produce any visible warnings. All tests pass except test_go_unknown_command.py, but that test also fails on a fresh clone of the upstream repository so I guess it's not related to my commits.

[scorphus]

Hi! Thanks for contributing! This is looking good already. Here are some change-requests to make it even better!

[scorphus]

Nice one. But what conditions does the ValueError get raised under? Can you please add a test case for that?

[chestnutcase]

I'm a little on the fence for this. I wrapped this in a try-except because I know list.index raises ValueError if the element is not found, however, I also know composer will always pad the output with some blank lines after the list of suggestions.

I thought about it and decided to not catch the exception – after all, it really is an exceptional case where composer would not output blank lines; it implies that composer has changed the format of its output messages and the exception bubbling to the user can explicitly signal for us to make a new fix for composer. I think it would be preferable over silent failure.

[scorphus]

Same applies here:

@for_app("composer")
def match(command):
    is_invalid_argument_exception = (
        "[InvalidArgumentException]" in command.output
    )
    is_unable_to_find_package = "Could not find package " in command.output
    suggestions_present = "Did you mean " in command.output
    return (
        is_invalid_argument_exception
        and is_unable_to_find_package
        and suggestions_present
    )

With added reformatting to make it more readable (Black FTW!)

[chestnutcase]

i switched to black and now flake8 complains about E203 because I made a function call within list slicing. it appears to be acknowledged by creators of black and they suggest ignoring it. see below.

[scorphus]

If only GitHub would show the comments in line-number order, other than chronological. Sorry if the review is confusing, please let me know if something is not clear.

[chestnutcase]

wow, thanks for the comprehensive code review! i'm currently occupied with midterm examinations at the moment so it'll take some time for me to follow up on your comments, i'll push my changes at the end of this week.

[chestnutcase]

Sorry for the delay. I've added in your requested changes, and also adopted Black as my code formatter.

Except for one problem: Black triggers E203 whitespace before : in line 31 of composer_not_package.py:

version_constraint = offending_script_param[len(wrong_package_name) :]

According to Black themselves, E203 should be ignored by flake. Perhaps we should check in a flake8 configuration file that follows their recommendation?

[scorphus]

Hey, thanks for keeping up! 👍

I'm happy that you adopted Black. But maybe we're going too far with it. Like changing lines that are not part of the fix – which I've just noticed. Things such as formatting that should be part of a different PR – I know it was in the middle of one or two of my suggestions, I didn't notice it back then, sorry for that.

Also, I've just noticed that this PR fixes a rule and introduces another. These should be at least two separate commits.

Do you think you can split the changes in separate commits, undo the lines that are not part of the fix/feature and update this PR? Otherwise, I could do that, if you don't mind. Authorship would be maintained, of course.

[chestnutcase]

This is my first code/feature related PR on a public repository so admittedly I'm not very familiar with best practices – are you saying I should add more commits that reverses (removes) the composer_not_package rule, then open another PR (perhaps I refork nvbn:master and rebase), and then re-add composer_not_package and open a separate PR?

I'm not very sure how to go about reversing selective lines of commits too; I don't use any Git GUI applications that can offer such a functionality.

[scorphus]

Sorry for the late reply, @chesnutcase.

Splitting into two different PRs would be ideal, but as we've come this far, splitting changes into two different commits is more than enough.

Regarding the changes introduced by the use of Black... On one hand, Black is great because it leaves no space for discussions about how to format code – it does it for you. Out of a sudden, you stop wasting time with everything related to code formatting. On the other hand, when it comes to a codebase that's not previously formatted by it, that may generate undesirable noise, such as some parts of this pull request. So it's up to the developer to include only the relevant changes.

Please LMK if I can make myself any clearer. (I often fail at that)

Thanks again for contributing and for sticking to it!