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

fix: assign to functions hash to make %x work #460

Merged
merged 1 commit into from
Feb 13, 2023

Conversation

psprint
Copy link
Contributor

@psprint psprint commented Jan 13, 2023

The useful %x prompt expansion can be used to get path to the executed script: 0=${(%):-%x}

Description

It is always the correct, expected value, even for autoloaded functions. However, it currently doesn't work with Zinit special fpath-pollution free autoload. This patch fixes this.

Motivation and Context

It is comfortable to get correct $0 even for autoloaded functions with:

0=${(%):-%x}

However to make it fully work under Zinit, it has to define its emulated autoload function with:

functions[func]=""

instead of:

eval "function func { …"

because the eval method returns wrong path at first function execution (returns path to name.plugin.zsh instead). That's the change of this PR.

How Has This Been Tested?

The change has to work, it's very isolated.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

The useful %x prompt expansion can be used to get path to the executed script:
0=${(%):-%x}

It is always the correct, expected value, even for autoloaded functions. However,
it currently doesn't work with Zinit special fpath-pollution free autoload.
This patch fixes this.
@vladdoster
Copy link
Member

I can't quite tell what this fixes. Could you share a screenshot?

@vladdoster
Copy link
Member

I thought it would fix the following issue at a cursory glance, but I guessed wrong.

Screenshot 2023-01-15 at 00 05 16

@psprint
Copy link
Contributor Author

psprint commented Jan 15, 2023

Here is the screenshot:

2023-01-15-131848_1904x789_scrot

@vladdoster
Copy link
Member

@psprint,

I'm still unclear on what this fixes. Does it improve a certain ice, command, or just zsh programming in general?

@psprint
Copy link
Contributor Author

psprint commented Jan 16, 2023

It allows to use better $0 retriever code than the currently recommended in plugin standard. So maybe yes it improves zsh programming in general. I'll be writing a patch to the standard if the zinit problem will be fixed.

@psprint
Copy link
Contributor Author

psprint commented Jan 29, 2023

Could the PR be merged? It`s a very local, isolated change, basically a bug fix. To provide more info on what it does:

  • the %x is a prompt expansion, which is used in a ${(%)…} expansion that enables the expansion outside the prompt,
  • so basically, %x would expand only in $PROMPT/$PS/etc. vars when printing prompt, while with (%) it expands in the place of use,
  • %x is a useful expansion, it returns path to the currently executed script or function,
  • so it is very useful to set $0 to the plg.plugin.zsh file, or even to any executed function,
  • however, the special autoload of Zinit is currently broken when first used the autoloaded func, when the function is loaded before first execution,
  • the PR fixes this, making %x return the path to the function correctly, even at first call.

Is everything clear? I'm holding back because %x isn't fixed in Zinit, before updating the plugin standard, it's 0=… recommendation there.

@vladdoster vladdoster merged commit b5338e0 into zdharma-continuum:main Feb 13, 2023
@psprint
Copy link
Contributor Author

psprint commented Feb 13, 2023

Thanks for merging. Could some other my PRs be merged? Like those mentioned in #448 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants