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

New time builtin cannot be detected as a builtin #6598

Closed
charlesbjohnson opened this issue Feb 13, 2020 · 3 comments
Closed

New time builtin cannot be detected as a builtin #6598

charlesbjohnson opened this issue Feb 13, 2020 · 3 comments
Assignees
Milestone

Comments

@charlesbjohnson
Copy link

charlesbjohnson commented Feb 13, 2020

Hi 👋

I've just upgraded to 3.1.0 and have just learned about the new time builtin. I'm running into a small issue, though, where the builtin command doesn't seem to be aware that time is now a builtin.

$ builtin -n | grep time; echo $status
$ 1
$ builtin -q time; echo $status
$ 1

As far as I can tell, this is because it was removed from src/builtin.cpp in 664d6fb. I'm not very familiar with this project, but would you accept a PR adding it back as a builtin_generic?

Edit: It's a little bit more work than I thought, see #6599.

@krobelus
Copy link
Member

krobelus commented Feb 14, 2020

Ugh this is a bit tricky since time is implemented as dedicated part of the grammar in order to support timing fish functions and blocks.
When adding time as generic builtin, the tests fail because a=b time is supposed to call the external command time, which it doesn't if it is a builtin. This behavior might not be too important, because one can use the more explicit a=b command time instead. So we could break it (changing the test), or add some workarounds like we do for and/or.

Also time -h and time --help are not supported currently. Similar story here.

@faho
Copy link
Member

faho commented Feb 14, 2020

I'm pretty sure we do something similar to what is required here for command.

@ridiculousfish
Copy link
Member

Right, these keywords have a dual life as both a keyword and a builtin. The builtin form is used for --help in particular; it doesn't need to do anything more than that.

@krobelus krobelus modified the milestones: fish-future, fish 3.2.0 Feb 23, 2020
@zanchey zanchey modified the milestones: fish 3.2.0, fish 3.1.1 Feb 24, 2020
zanchey pushed a commit that referenced this issue Feb 24, 2020
`a=b time foo` will no longer call an external `time` command
(like it does in bash).

Fixes #6598

(cherry picked from commit 7ef7f93)
@krobelus krobelus added this to the fish 3.1.1 milestone Feb 24, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants