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(init)!: allow turning off aliases for libs and plugins #11550

Merged
merged 6 commits into from Apr 3, 2023

Conversation

mcornella
Copy link
Member

@mcornella mcornella commented Mar 8, 2023

Standards checklist:

  • The PR title is descriptive.
  • The PR doesn't replicate another PR which is already open.
  • I have read the contribution guide and followed all the instructions.
  • The code follows the code style guide detailed in the wiki.
  • The code is mine or it's from somewhere with an MIT-compatible license.
  • The code is efficient, to the best of my ability, and does not waste computer resources.
  • The code is stable and I have tested it myself, to the best of my abilities.
  • If the code introduces new aliases, I provide a valid use case for all plugin users down below.

Proof of concept work for backing up and restoring aliases on libs and plugins that have them turned off by the user via ztyle:

zstyle ':omz:*' aliases no
zstyle ':omz:lib:*' aliases no
zstyle ':omz:plugins:*' aliases no
zstyle ':omz:plugins:git' aliases no

Aliases could also be selectively enabled for specific components, such as:

zstyle ':omz:plugins:*' aliases no
zstyle ':omz:plugins:git' aliases yes

Work in collaboration with @carlosala.

Fixes #9414

@ohmyzsh ohmyzsh bot added the Area: init Issue or PR related to the initializer label Mar 8, 2023
@carlosala
Copy link
Member

We can see in #10510 what does this PR closes also 👌🏻

@ohmyzsh ohmyzsh deleted a comment from Paolaarellano665 Mar 11, 2023
@ohmyzsh ohmyzsh deleted a comment from Paolaarellano665 Mar 11, 2023
mcornella and others added 2 commits April 2, 2023 13:49
Proof of concept work for backing up and restoring aliases on
libs and plugins that have them turned off by the user via
ztyle:

  zstyle ':omz:*' aliases no
  zstyle ':omz:lib:*' aliases no
  zstyle ':omz:plugins:*' aliases no
  zstyle ':omz:plugins:git' aliases no

Co-authored-by: Carlo Sala <carlosalag@protonmail.com>
@carlosala
Copy link
Member

Just a quick reminder here, when merging this we should soft-deprecate

zstyle -T ':omz:directories' aliases || return 0
as is not following the ':omz:lib:<file> structure and document properly also that in README (removing also
### Remove directories aliases
)

@ohmyzsh ohmyzsh bot added the Type: documentation Documentation issue or Pull Request label Apr 2, 2023
Copy link
Member

@carlosala carlosala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 😉

@carlosala
Copy link
Member

After testing with https://github.com/romkatv/zsh-bench, I cannot see a difference while removing all aliases and leaving all of them. It seems that the way we are keeping and restoring aliases has no performance issues.
I think we can merge this, and see how the community receives it.
I'm quite happy we finally sorted it :)

@mcornella
Copy link
Member Author

I'm currently debugging why setting galiases does not remove the global aliases, don't merge it yet.

@carlosala
Copy link
Member

At least from the documentation perspective, it should work exactly the same. Even though, if it's giving issues, I only see global aliases defined here and here. We could have some workaround there 👍🏻

@mcornella
Copy link
Member Author

mcornella commented Apr 2, 2023

OK it looks like the same happens with aliases and the rest of special parameters when we're doing array=() (empty array).

When that happens, the resulting hash table does not actually get modified, I don't know exactly why by looking at the code: https://github.com/zsh-users/zsh/blob/c006d760/Src/Modules/parameter.c#L1767-L1807

I have pushed a version that uses unalias directly in a "smarter" way to avoid many calls, but I don't know if it is actually slower. Could you try it out with zsh-bench again?


EDIT: it looks like the culprit is that the hashtable is actually empty, so this check matches and nothing is done: https://github.com/zsh-users/zsh/blob/c006d7609703afcfb2162c36d4f745125df45879/Src/Modules/parameter.c#L1773-L1774

@carlosala
Copy link
Member

I think the code is a bit more difficult to read. What about adding always a garbage alias, maybe alias __omz_tmp=<whatever>. Then we are sure the hash table is never empty (and we could use unalias only for that).
What do you think?

@mcornella
Copy link
Member Author

That looks like a hack, I have instead pushed a change that doesn't resort to weird tricks, a simple unalias was enough. It should not be less fast, or even faster.

Copy link
Member

@carlosala carlosala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it's more readable I'd say, LGTM!
Incredible job Marc😉

@ohmyzsh ohmyzsh bot added the Area: core Issue or PR related to core parts of the project label Apr 3, 2023
@mcornella mcornella changed the title feat(init): allow turning off aliases for libs and plugins feat(init)!: allow turning off aliases for libs and plugins Apr 3, 2023
@mcornella mcornella merged commit 1ad167d into ohmyzsh:master Apr 3, 2023
3 checks passed
@mcornella mcornella deleted the poc/disable-aliases branch April 3, 2023 21:14
@mcornella
Copy link
Member Author

Shipped! 🚀 Thank you for the review!

hron84 pushed a commit to hron84/oh-my-zsh that referenced this pull request Apr 11, 2023
…11550)

BREAKING CHANGE: the previous zstyle setting to disable `lib/directories.zsh` aliases has
been changed to the new syntax: `zstyle ':omz:lib:directories' aliases no`. See
https://github.com/ohmyzsh/ohmyzsh#skip-aliases to see other ways you can use this setting.
    
Co-authored-by: Carlo Sala <carlosalag@protonmail.com>
carlosala added a commit to carlosala/ohmyzsh that referenced this pull request May 1, 2023
Fix regression introduced in ohmyzsh#11550. If an existing alias was present in
the moment of sourcing, and oh-my-zsh aliases were disabled for that
file, it'd be overwritten aswell. See ohmyzsh#11658.
carlosala added a commit that referenced this pull request May 1, 2023
Fix regression introduced in #11550. If an existing alias was present in
the moment of sourcing, and oh-my-zsh aliases were disabled for that
file, it'd be overwritten aswell. See #11658.
hron84 pushed a commit to hron84/oh-my-zsh that referenced this pull request May 2, 2023
Fix regression introduced in ohmyzsh#11550. If an existing alias was present in
the moment of sourcing, and oh-my-zsh aliases were disabled for that
file, it'd be overwritten aswell. See ohmyzsh#11658.
kis87988 pushed a commit to kis87988/ohmyzsh that referenced this pull request May 21, 2023
…11550)

BREAKING CHANGE: the previous zstyle setting to disable `lib/directories.zsh` aliases has
been changed to the new syntax: `zstyle ':omz:lib:directories' aliases no`. See
https://github.com/ohmyzsh/ohmyzsh#skip-aliases to see other ways you can use this setting.
    
Co-authored-by: Carlo Sala <carlosalag@protonmail.com>
kis87988 pushed a commit to kis87988/ohmyzsh that referenced this pull request May 21, 2023
Fix regression introduced in ohmyzsh#11550. If an existing alias was present in
the moment of sourcing, and oh-my-zsh aliases were disabled for that
file, it'd be overwritten aswell. See ohmyzsh#11658.
nbaronov pushed a commit to nbaronov/oh-my-zsh that referenced this pull request Aug 3, 2023
…11550)

BREAKING CHANGE: the previous zstyle setting to disable `lib/directories.zsh` aliases has
been changed to the new syntax: `zstyle ':omz:lib:directories' aliases no`. See
https://github.com/ohmyzsh/ohmyzsh#skip-aliases to see other ways you can use this setting.
    
Co-authored-by: Carlo Sala <carlosalag@protonmail.com>
nbaronov pushed a commit to nbaronov/oh-my-zsh that referenced this pull request Aug 3, 2023
Fix regression introduced in ohmyzsh#11550. If an existing alias was present in
the moment of sourcing, and oh-my-zsh aliases were disabled for that
file, it'd be overwritten aswell. See ohmyzsh#11658.
cschuyle pushed a commit to cschuyle/ohmyzsh that referenced this pull request Apr 18, 2024
Fix regression introduced in ohmyzsh#11550. If an existing alias was present in
the moment of sourcing, and oh-my-zsh aliases were disabled for that
file, it'd be overwritten aswell. See ohmyzsh#11658.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Issue or PR related to core parts of the project Area: init Issue or PR related to the initializer Type: documentation Documentation issue or Pull Request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add option to disable ohmyzsh's aliases
3 participants