Fix corrected command not being saved into fish history

[djh82]

This should fix #1027

[nvbn]

Changes look good for me, but it seems like it breaks e2e tests - https://travis-ci.org/nvbn/thefuck/jobs/630646284#L3756

[djh82]

Yes, I need to get round to fixing those. Having trouble running the end to end tests locally atm.

[scorphus]

That missing \n is the reason those integration tests are failing.

As far as the issue is concerned, I am not entirely sure yet whether this is the best way to solve it. It does not go very well with some usage scenarios such as post_match rules and a new edit command feature I've been working on.

[djh82]

@scorphus happy for you to suggest further changes; or reject this entirely if it doesn't fit with your future changes. This is what I'm currently using at work, since the currently implemented method for updating the fish history simply doesn't work, and it's a bit frustrating, so thought I'd share!

[scorphus]

Let me have a look at this again.

[scorphus]

So, there are two reasons I think we should avoid changing the shell function. The main one is backward compatibility. A considerable number of users have the function hardcoded either in their config.fish or in some functions/fuck.fish. I've seen a lot of cases like that. Even some issues have that as a suggestion to improve the loading time of the shell.

Another reason is that it does not support a new feature I'm bringing together, which is the “edit command.” For that, I'd like to avoid having the commandline instruction added to the history.

That said, I think the commandline part of this PR should be moved into shell. I already have a working CL, which I'll submit shortly afterward as a new commit on top of yours. Then, you can review it.

Sounds good?

[djh82]

Sounds good to me! 👍

[scorphus]

Hey, @davidhart82! I went ahead – as we usually squash commits by the same author that represent one single, “atomic” change – and squashed the two commits you had. So, authorship is preserved.

I'm looking forward to reading your thoughts on my proposed changes.

Thanks!

[djh82]

Hey @scorphus, changes look good to me! Much neater given I had no idea folk hardcode the function directly in config.fish (I'm clearly spoiled with the power of my workstation ;)).

I'll give it a test and let you know how I get on...

[djh82]

This only appears to have failed since the coverage has dropped by 0.01%...

Though, having tested this, although it works, it still stores the call to 'fuck' in the history. I wonder whether we want to search and delete those too?

[scorphus]

Looking good! I'll have time to review it over the weekend. Super!

[tonytamps]

Sorry to be the thread necromancer. Wondering if there is a reason 3.31 was never released?
There are a couple things I've been waiting a long while for and this one I've been desperate for since the issue was raised.

[scorphus]

Thanks for the reminder, @tonytamps! There're a few more issues/PRs that need to be solved/merged. All of the issues are already WIP. We're getting there!

[tonytamps]

All good. Thank you.

[scorphus]

BTW, I'm not entirely happy with how we managed to solve this issue, I guess I'll try some other approach(es).

[mainrs]

Is there anything that can be done to push this forward? The issue is still present. I've read the diff but I am not sure what the exact problem was.

[septatrix]

I am not sure since when this issue exists but it also occured to me that fish complaints because the history merge command does not take any arguments so I would be very glad if this could be merged soon as it looks like a good fix to me

[scorphus]

@septatrix: that issue (#1215) is fixed and released in 3.32. This PR addresses another issue.