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

make it easy to learn where a function came from #3295

Closed
krader1961 opened this Issue Aug 9, 2016 · 16 comments

Comments

Projects
None yet
5 participants
@krader1961
Contributor

krader1961 commented Aug 9, 2016

We regularly get questions about unexpected behavior whose investigation involves verifying that the expected fish functions are being loaded. For example, issue #3210. It should be much easier to answer that question. A first approximation solution is to do something like

for fn_dir in $fish_function_path
    test -r $fn_dir/fish_vi_key_bindings.fish
    and echo $fn_dir/fish_vi_key_bindings.fish
end

The problem with that solution is there is no guarantee the function came from the corresponding autoload script.

I propose two enhancements. First, augment the builtin functions command to take a function name and report where it came from. Second, augment the type function to report all the corresponding autoload scripts using something like the snippet above. It seems like type -P $func_name should be enhanced to report the first autoload script found or all of them if -a is also specified. It's not clear why the current behavior only deals with external commands and not autoloaded functions. If changing type -P to also handle autoloaded functions is too confusing, or likely to break existing uses, then we could add an analogous -F option to find autoload scripts rather than external commands.

@krader1961 krader1961 added this to the fish-future milestone Aug 9, 2016

@krader1961 krader1961 self-assigned this Aug 9, 2016

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Aug 9, 2016

Member

We already track which functions are autoloaded. It would be straightforward to report that fact in the functions output.

Member

ridiculousfish commented Aug 9, 2016

We already track which functions are autoloaded. It would be straightforward to report that fact in the functions output.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Aug 10, 2016

Member

This would enable a funced that can optionally work with originals, and easy checking for un-funcsaved functions (I'd like to have mine list them out at exit).

Member

floam commented Aug 10, 2016

This would enable a funced that can optionally work with originals, and easy checking for un-funcsaved functions (I'd like to have mine list them out at exit).

krader1961 added a commit to krader1961/fish-shell that referenced this issue Jan 10, 2017

implement means to learn about a function source
This implements a way to use the `functions` command to perform
introspection to learn about the characteristics of a function. Such as
where it came from.

Partial fix for #3295
@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jan 10, 2017

Contributor

I've created PR #3718 that implements the enhancement described above to the functions command. It adds -f / --file flags. However I am not entirely happy with those flag names since the verbose output includes information not directly related to the file name where the function was defined. Specifically, whether or not the function was defined with the --no-scope-shadowing flag. Opinions for alternative flag names or otherwise requiring the non-source related output to be done under other flags is welcome.

Contributor

krader1961 commented Jan 10, 2017

I've created PR #3718 that implements the enhancement described above to the functions command. It adds -f / --file flags. However I am not entirely happy with those flag names since the verbose output includes information not directly related to the file name where the function was defined. Specifically, whether or not the function was defined with the --no-scope-shadowing flag. Opinions for alternative flag names or otherwise requiring the non-source related output to be done under other flags is welcome.

krader1961 added a commit to krader1961/fish-shell that referenced this issue Jan 10, 2017

implement means to learn about a function source
This implements a way to use the `functions` command to perform
introspection to learn about the characteristics of a function. Such as
where it came from.

Partial fix for #3295
@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Jan 10, 2017

Member

However I am not entirely happy with those flag names since the verbose output includes information not directly related to the file name where the function was defined.

I can see that. You've implemented this in a function called "show_function_details", so how about "--details" or "--show-details"? Of course it couldn't use "-d" since that's already taken for changing the description, so it would have to be "-D". Or simply call it "--verbose" and either support passing it twice (functions -vv somefunc) or print all the details always.

Or since this is metadata, something with that term?

Member

faho commented Jan 10, 2017

However I am not entirely happy with those flag names since the verbose output includes information not directly related to the file name where the function was defined.

I can see that. You've implemented this in a function called "show_function_details", so how about "--details" or "--show-details"? Of course it couldn't use "-d" since that's already taken for changing the description, so it would have to be "-D". Or simply call it "--verbose" and either support passing it twice (functions -vv somefunc) or print all the details always.

Or since this is metadata, something with that term?

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jan 10, 2017

Contributor

A -m / --metadata flag was my first thought too but I was concerned it was too obscure a term. I don't think we should overload --verbose in that manner since it is typically used as a modifier to some other action. So I think we'll end up using either --metadata or --details.

I'm also considering putting the metadata in a comment at the top of the functions $funcname output. Thus functions --metadata $funcname then means to output only the metadata and not the body of the function. The metadata output only allows a single function name since the intended use case wouldn't normally pass more than one name and we can't add to the --verbose output in the future if we allow more than one function name. On the other hand we currently allow displaying the definition of more than one function per functions invocation. Thoughts?

Contributor

krader1961 commented Jan 10, 2017

A -m / --metadata flag was my first thought too but I was concerned it was too obscure a term. I don't think we should overload --verbose in that manner since it is typically used as a modifier to some other action. So I think we'll end up using either --metadata or --details.

I'm also considering putting the metadata in a comment at the top of the functions $funcname output. Thus functions --metadata $funcname then means to output only the metadata and not the body of the function. The metadata output only allows a single function name since the intended use case wouldn't normally pass more than one name and we can't add to the --verbose output in the future if we allow more than one function name. On the other hand we currently allow displaying the definition of more than one function per functions invocation. Thoughts?

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jan 14, 2017

Contributor

Oops! The text that used to be this comment was meant for a different issue.

Contributor

krader1961 commented Jan 14, 2017

Oops! The text that used to be this comment was meant for a different issue.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jan 16, 2017

Contributor

I've changed my mind about augmenting the type command. At least as part of addressing this issue. So I'm going to consider this resolved once the functions command can report metadata such as where the function came from.

Contributor

krader1961 commented Jan 16, 2017

I've changed my mind about augmenting the type command. At least as part of addressing this issue. So I'm going to consider this resolved once the functions command can report metadata such as where the function came from.

@krader1961 krader1961 modified the milestones: next-minor, fish-future Jan 21, 2017

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Apr 27, 2017

Member

I'm very behind, but is it too late to rename this to --details rather than --metadata? I think the former is lot simpler.

Member

zanchey commented Apr 27, 2017

I'm very behind, but is it too late to rename this to --details rather than --metadata? I think the former is lot simpler.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Apr 27, 2017

Contributor

is it too late to rename this to --details rather than --metadata?

Yes, since this feature is in the 2.5.0 release. Granted it is very unlikely anyone is yet using this feature but since the change would be purely aesthetic (i.e., gratuitous) it's pretty hard to justify.

Contributor

krader1961 commented Apr 27, 2017

is it too late to rename this to --details rather than --metadata?

Yes, since this feature is in the 2.5.0 release. Granted it is very unlikely anyone is yet using this feature but since the change would be purely aesthetic (i.e., gratuitous) it's pretty hard to justify.

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Apr 28, 2017

Member

2e38cf2 isn't in 2.5.0, so I think we've still got the chance.

Member

zanchey commented Apr 28, 2017

2e38cf2 isn't in 2.5.0, so I think we've still got the chance.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Apr 28, 2017

Contributor

Yep, you're right, @zanchey. I had synced my 2.5.0 client to the wrong commit because I did it by hand rather than git checkout 2.5.0 ☹️

As for --details or --metadata I don't have a strong opinion. As a SW engineer I'm inclined to prefer the latter since it more clearly communicates the purpose of the flag. But I can see how it might be unclear to more casual programmers. Too, I don't think --details is "simpler" than --metadata. Just possibly easier to grasp by someone without a CS background. Also, note that --metadata allows us to use the natural -m short form. Whereas --details will have to use something other than -d as its short flag equivalent.

We're going to need feedback from a couple more people since we each prefer different terms and it's not obvious that one of them, or perhaps a different term we haven't thought of, is superior.

Contributor

krader1961 commented Apr 28, 2017

Yep, you're right, @zanchey. I had synced my 2.5.0 client to the wrong commit because I did it by hand rather than git checkout 2.5.0 ☹️

As for --details or --metadata I don't have a strong opinion. As a SW engineer I'm inclined to prefer the latter since it more clearly communicates the purpose of the flag. But I can see how it might be unclear to more casual programmers. Too, I don't think --details is "simpler" than --metadata. Just possibly easier to grasp by someone without a CS background. Also, note that --metadata allows us to use the natural -m short form. Whereas --details will have to use something other than -d as its short flag equivalent.

We're going to need feedback from a couple more people since we each prefer different terms and it's not obvious that one of them, or perhaps a different term we haven't thought of, is superior.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Apr 28, 2017

Member

I prefer --details.

Member

floam commented Apr 28, 2017

I prefer --details.

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Apr 28, 2017

Member

I think --details/-D would work - mdadm uses --detail/-D.

Member

zanchey commented Apr 28, 2017

I think --details/-D would work - mdadm uses --detail/-D.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Apr 28, 2017

Contributor

I think --details/-D would work - mdadm uses --detail/-D.

Okay, but many other commands use --metadata. Just cd share/completions; grep metadata *. A similar search for details returns roughly the same number of matches. Too, if you run man mdaddm you'll find that the word metadata appears multiple times such as in the context of its --examine flag.

Both of us are obviously arguing from extremely limited data sets. So I think it makes more sense to argue from first principals rather than what is most popular. As I said earlier I think metadata is clearer as to the purpose of the flag compared to details. But that obviously reflects my bias as a SW engineer.

At the end of the day I think we're arguing whether it is "tomato" or "tomatoe". What the flag causes to be output is fundamentally metadata as the term is used by most people in the IT industry. But if that is deemed too esoteric, especially for people for whom English is a second language, then --details or something like it may be a better choice.

Contributor

krader1961 commented Apr 28, 2017

I think --details/-D would work - mdadm uses --detail/-D.

Okay, but many other commands use --metadata. Just cd share/completions; grep metadata *. A similar search for details returns roughly the same number of matches. Too, if you run man mdaddm you'll find that the word metadata appears multiple times such as in the context of its --examine flag.

Both of us are obviously arguing from extremely limited data sets. So I think it makes more sense to argue from first principals rather than what is most popular. As I said earlier I think metadata is clearer as to the purpose of the flag compared to details. But that obviously reflects my bias as a SW engineer.

At the end of the day I think we're arguing whether it is "tomato" or "tomatoe". What the flag causes to be output is fundamentally metadata as the term is used by most people in the IT industry. But if that is deemed too esoteric, especially for people for whom English is a second language, then --details or something like it may be a better choice.

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Apr 28, 2017

Member

Yes, I think it's too esoteric.

I've looked at the information it provides and I wonder if --source would be better, although overloading that word is a bit of a concern.

Member

zanchey commented Apr 28, 2017

Yes, I think it's too esoteric.

I've looked at the information it provides and I wonder if --source would be better, although overloading that word is a bit of a concern.

krader1961 added a commit to krader1961/fish-shell that referenced this issue May 1, 2017

rename --metadata to --details
Discussion in issue #3295 resulted in a decisions to rename the
functions --metadata flag to --details.

This also fixes a bug in the definition of the short flags for the
`functions` command. The `-e` flag does not take an argument and
therefore should not be defined as `e:`. Notice that the long form,
`--erase`, specifies `no_argument`. This discrepency happened to work
due to a quirk of how the flag parsing loop was written.
@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 May 1, 2017

Contributor

Since I had only a very weak preference for --metadata I've pushed commit 6b1c939 to change it to --details (and -D rather than -m). Anyone who argues for a different flag name for this feature is going after this point in time is going to get a cold shoulder.

Contributor

krader1961 commented May 1, 2017

Since I had only a very weak preference for --metadata I've pushed commit 6b1c939 to change it to --details (and -D rather than -m). Anyone who argues for a different flag name for this feature is going after this point in time is going to get a cold shoulder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment