thefuck
thefuck copied to clipboard
nvbn
Fix corrected command not being saved into fish history
Changes look good for me, but it seems like it breaks e2e tests - https://travis-ci.org/nvbn/thefuck/jobs/630646284#L3756
Yes, I need to get round to fixing those. Having trouble running the end to end tests locally atm.
@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!
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?
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!
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...
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?
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.
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!
BTW, I'm not entirely happy with how we managed to solve this issue, I guess I'll try some other approach(es).
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.
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
