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

eval scope #4443

Closed
mqudsi opened this issue Oct 3, 2017 · 8 comments
Closed

eval scope #4443

mqudsi opened this issue Oct 3, 2017 · 8 comments
Labels
bug
Milestone

Comments

@mqudsi
Copy link
Contributor

@mqudsi mqudsi commented Oct 3, 2017

Is the following a bug or a feature?

eval set -l foo bar
if not echo "bar" | string match -q $foo
    echo Assertion failed!
end

eval isn't explicitly documented as creating a scope.

(note that this "feature" caused some consternation in the implementing of #4442)

@mqudsi mqudsi added the question label Oct 3, 2017
@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Oct 3, 2017

See #565 (comment)

Still not clear on whether it's a bug or not, though.

@mqudsi mqudsi added RFC and removed question labels Oct 10, 2017
@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Mar 9, 2018

@ridiculousfish any thoughts on this?

@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Mar 19, 2018

It's a bug due to eval's implementation as a function. I thought we had another issue tracking this.

@ridiculousfish ridiculousfish added this to the fish-future milestone Mar 19, 2018
@mqudsi mqudsi added bug and removed RFC labels Mar 30, 2018
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Apr 11, 2019
`eval` has always been implemented as a function, which was always a bit
of a hack that caused some issues such as triggering the creation of a
new scope. This turns `eval` into a decorator.

Closes fish-shell#4443.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Apr 11, 2019
@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Apr 11, 2019

This should work correctly now.

@faho
Copy link
Member

@faho faho commented Jun 21, 2019

Okay, so the eval builtin doesn't create a new scope, but source still does?

I think we might want to change it in source as well?

@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Aug 26, 2019

I keep remembering I need to reply to this and then forgetting to do so.

TBH, I like that source has its own scope -- since you're typically editing the files out-of-band and they're expected to play nice regardless of how they're loaded, etc. I feel it would be counter-intuitive for source to use the existing scope. I'd prefer just officially changing that from "bug" to "feature" and explicitly documenting that source creates a new scope.

zanchey added a commit that referenced this issue Sep 17, 2019
@zanchey
Copy link
Member

@zanchey zanchey commented Sep 17, 2019

Noted in the docs.

@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Sep 18, 2019

Thanks, @zanchey!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug
Projects
None yet
Development

No branches or pull requests

4 participants