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

Implement d; and d, #7516

Merged
merged 1 commit into from Nov 29, 2020
Merged

Implement d; and d, #7516

merged 1 commit into from Nov 29, 2020

Conversation

Aster89
Copy link
Contributor

@Aster89 Aster89 commented Nov 29, 2020

With this change I fix address one of the several issues raised in #4019 in a pretty obvious way: some bindings is missing and I'm adding two of them.

As discussed there, this is not the best way to face the inadequacy of the vi-mode that fish offers, but at the moment this is the most I can do (in terms of my time/knowledge/understanding of the existing code/programmer experience). I hope I'll be able to to something better in the future, beside possibly fixing similar issues. (For instance y; and y, should be easy to write too.)

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

@faho faho added this to the fish 3.2.0 milestone Nov 29, 2020
@faho
Copy link
Member

faho commented Nov 29, 2020

With this change I fix issue #4019 in a pretty obvious way: the binding is missing and I'm adding it.

This does not fix issue #4019. It solves a specific subset, which is d; and d,.

The commit message mentions dw, which it doesn't touch in any way.

It's probably a good change regardless, but the commit message is flat out wrong and misleading. Please reword it and we'll merge!

Unless, of course, you're planning on adding more to it.

@Aster89 Aster89 changed the title Address issue #4019 Implement d; and d, Nov 29, 2020
@Aster89
Copy link
Contributor Author

Aster89 commented Nov 29, 2020

Sorry, man, I'm trying to contribute in a useful way. I'll pay more attention next time when setting a title.

As regards other changes, I'll try to make some more in the future, but doing a bit now and getting it merged is a first step.

By the way, I've received an email about an unsuccessful job on macOS. I'll give a look at it when I can, ok?

@faho
Copy link
Member

faho commented Nov 29, 2020

Sorry, man, I'm trying to contribute in a useful way. I'll pay more attention next time when setting a title.

Oh, terribly sorry, that probably came across as needlessly mean.

I was just trying to stop people from thinking this would fix all of their worries regarding vi-mode, and in trying to get that across quickly was overzealous in my phrasing. Sorry!

By the way, I've received an email about an unsuccessful job on macOS. I'll give a look at it when I can, ok?

No need, I already took a look. We recently switched to Github Actions and the tests are a bit flakey there (which is always an issue since we need to test interactivity, so we do have some timeouts, and common CI systems are super resource constrained - you often don't even get a keypress within 100ms).

I reran them and they passed.


As for the commit itself? Merging right now. (I forgot that I could also change the commit message)

@faho faho merged commit 2a86099 into fish-shell:master Nov 29, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants