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

feat(zsh): update widget names #1631

Merged
merged 1 commit into from Jan 29, 2024
Merged

Conversation

akinomyoga
Copy link
Contributor

This is another suggestion. See the commit message.

For more contexts, the following is the list of how the other zsh frameworks name their widgets:

Also, it should be noted that ZLE also prepares the special widget names zle-* for the users to customize the behavior. The most popular namespacing seems to be <ProjectName>-*.

@ellie
Copy link
Member

ellie commented Jan 26, 2024

I'm happy with the change, but would it be possible to have aliases to the existing names?

If we rename them without warning, some setups will break. One release is enough warning imo, we can remove the aliases after that

@akinomyoga
Copy link
Contributor Author

I'm happy with the change, but would it be possible to have aliases to the existing names?

If we rename them without warning, some setups will break.

Maybe I missed something, but I already left the old names (as described also in the commit message), so the existing setups shouldn't break. Or does it need to be a real alias in the shell's sense?

Or should I also leave the new ones _atuin{,_up}_search_vi{cmd,ins}_widget? These are recently added and did not exist in the release version, so I dropped those new names and only left _atuin{,_up}_search_widget, which exist in the release version.

One release is enough warning imo, we can remove the aliases after that

I didn't intend to remove it soon, but if you'd like the warning message, I can add the warning message.

The current widget names for Zsh start with "_", which gives an
impression to users that those widgets are internal API and should not
be bound by the users.  However, we actually instruct users to set up
custom keybindings by specifying them to bindkey.

In other shells, a separate namespace for widgets are not prepared, so
we want to prefix "_" to shell function names to tell the users that
these are not the commands that are supposed to be called from the
command line.  However, the widget names are separated in their own
namespace in Zsh, so we do not have to isolate them by prefixing "_".
In fact, other frameworks such as `fzf` define widgets with names not
starting with "_".

In this patch, we update the widget names to have the form "atuin-*".
The old widget names that existed in the release version <= 17.2.1 are
left for compatibility.
@akinomyoga
Copy link
Contributor Author

I cleaned up slashes in docs that escaped _ before. Now we removed _ so don't have to keep the backslashes.

@ellie
Copy link
Member

ellie commented Jan 29, 2024

Maybe I missed something, but I already left the old names (as described also in the commit message), so the existing setups shouldn't break. Or does it need to be a real alias in the shell's sense?

No you're right, it's I that misread it! Thank you 🙏

Copy link
Member

@ellie ellie 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! Would it be possible to get a PR to the main docs too please? 🙏

@ellie ellie merged commit 2ef5169 into atuinsh:main Jan 29, 2024
8 checks passed
@akinomyoga
Copy link
Contributor Author

Thanks!

Thank you! Would it be possible to get a PR to the main docs too please? 🙏

Sure! I'm not able to create it now, but I'll later submit it (probably after a few or several hours).

@akinomyoga akinomyoga deleted the zsh-widget-name branch January 29, 2024 10:36
akinomyoga added a commit to akinomyoga/atuin-docs that referenced this pull request Jan 29, 2024
@akinomyoga
Copy link
Contributor Author

Thank you! Would it be possible to get a PR to the main docs too please? 🙏

Sure! I'm not able to create it now, but I'll later submit it (probably after a few or several hours).

atuinsh/docs#19

ellie pushed a commit to atuinsh/docs that referenced this pull request Jan 29, 2024
Commit b8c6b9a corresponds to the widget name change introduced in
atuinsh/atuin#1631. Also, the current
documentation diverges from the current implementation. I updated the
widgets for the <kbd>up</kbd> key in commit 6e1f3df. Also, the
descriptions for the newly added widgets for `keymap_mode`
(atuinsh/atuin#1570) are added in commit
705214a.

Since the version dependence of documentation can confuse people as in
atuinsh/atuin#1625, I decided to add a note on
the version dependence and also a required version for each feature. I
personally think it is useful to maintain in the documentation the
minimal version requirement for each feature, but this might be the
perspective of a developer who needs to consider the compatibility of
products with arbitrary versions of other products. If you have a
different preference, please tell me that.

There is an extra commit 33a1d6d. Sorry, this is unrelated to the widget
name but a tiny clarification of a description I added in my previous PR
#10. If I should submit a change in a separate PR, please tell me that.
It's a small change, but I'll create a separate one if you'd prefer it.
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

2 participants