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

fish and fish_indent assign different meaning to `-d` flag #3191

Closed
krader1961 opened this Issue Jul 2, 2016 · 5 comments

Comments

Projects
None yet
4 participants
@krader1961
Contributor

krader1961 commented Jul 2, 2016

I recently modified the fish_key_reader source to support the -d and -D flags (and associated long names) to be compatible with fish. I just noticed that fish_indent uses -d for a different purpose. Neither does it support -D. That's extremely confusing. This issue tracks adding support to fish_indent for the same behavior that fish assigns to those flags. Move its current -d behavior to a different short flag.

@krader1961 krader1961 added the cleanup label Jul 2, 2016

@krader1961 krader1961 added this to the fish-future milestone Jul 2, 2016

@krader1961 krader1961 self-assigned this Jul 2, 2016

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Jul 2, 2016

Member

This isn't an often-used thing - I can't find any use in our codebase, and I have no idea how I should interpret what it prints.

How about removing the short option completely?

Member

faho commented Jul 2, 2016

This isn't an often-used thing - I can't find any use in our codebase, and I have no idea how I should interpret what it prints.

How about removing the short option completely?

@siteshwar

This comment has been minimized.

Show comment
Hide comment
@siteshwar

siteshwar Jul 2, 2016

Member

-d flag for fish_indent seems to be used for debugging purpose only. However output format is a bit arcane and it seems this option is not meant for end users. We should probably enable this option for development builds only and improve the output format to be more readable.

Member

siteshwar commented Jul 2, 2016

-d flag for fish_indent seems to be used for debugging purpose only. However output format is a bit arcane and it seems this option is not meant for end users. We should probably enable this option for development builds only and improve the output format to be more readable.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jul 2, 2016

Contributor

The fish_indent -d flag isn't for debugging as much as providing insight into the state of the fish parser. I implemented it when I was working on changes such as commit 4ff8e6e and 59f0261. Anyone working on enhancements to the program is welcome to improve how that information is presented but it will, due to its very nature, never be easy to read or understand. I don't think there is much point in complicating the implementation by making it only accessible in a development build. However, as @faho suggested, I'm going to not assign it a new short flag. Since it is only for developers and it will be used very rarely having the feature be accessible only via a long flag is acceptable.

Contributor

krader1961 commented Jul 2, 2016

The fish_indent -d flag isn't for debugging as much as providing insight into the state of the fish parser. I implemented it when I was working on changes such as commit 4ff8e6e and 59f0261. Anyone working on enhancements to the program is welcome to improve how that information is presented but it will, due to its very nature, never be easy to read or understand. I don't think there is much point in complicating the implementation by making it only accessible in a development build. However, as @faho suggested, I'm going to not assign it a new short flag. Since it is only for developers and it will be used very rarely having the feature be accessible only via a long flag is acceptable.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Jul 2, 2016

Member

When would one use this instead of the __fish_parse builtin? (it sure is prettier and doing roughly the same thing... maybe the goals are different.)

> echo "echo x" | __fish_parse

 0 -  0  job_list <2 children>  [0, 7]
 1 -  1    job <3 children>  [0, 6]
 2 -  3      statement <1 children>  [0, 6]
 3 -  6        decorated_statement <1 children>  [0, 6]
 4 -  7          plain_statement <2 children>  [0, 6]
 5 -  8            token_string: "echo"
 6 -  9          arguments_or_redirections_list <2 children>  [5, 1]
 7 - 10            argument_or_redirection <1 children>  [5, 1]
 8 - 12              symbol_argument <1 children>  [5, 1]
 9 - 13                token_string: "x"
10 - 11          arguments_or_redirections_list  [no src]
11 -  4      job_continuation  [no src]
12 -  5      optional_background  [no src]
13 -  2  job_list <2 children>  [6, 1]
14 - 14    token_end  [6, 1]
15 - 15  job_list  [no src]

> echo "echo x" | fish_indent -d

{off    0, len    7, indent  0, kw none, symbol_job_list} [ |echo x
| ]
{off    0, len    6, indent  0, kw none, symbol_job} [ |echo x|\cJ]
{off    0, len    6, indent  0, kw none, symbol_statement} [ |echo x|\cJ]
{off    0, len    6, indent  0, kw none, symbol_decorated_statement} [ |echo x|\cJ]
{off    0, len    6, indent  0, kw none, symbol_plain_statement} [ |echo x|\cJ]
{off    0, len    4, indent  0, kw none, parse_token_type_string} [ |echo| ]
{off    5, len    1, indent  0, kw none, symbol_arguments_or_redirections_list} [ |x|\cJ]
{off    5, len    1, indent  0, kw none, symbol_argument_or_redirection} [ |x|\cJ]
{off    5, len    1, indent  0, kw none, symbol_argument} [ |x|\cJ]
{off    5, len    1, indent  0, kw none, parse_token_type_string} [ |x|\cJ]
{off    6, len    0, indent  0, kw none, symbol_arguments_or_redirections_list} [x||\cJ]
{off    6, len    0, indent  0, kw none, symbol_job_continuation} [x||\cJ]
{off    6, len    0, indent  0, kw none, symbol_optional_background} [x||\cJ]
{off    6, len    1, indent  0, kw none, symbol_job_list} [x|
| ]
{off    6, len    1, indent  0, kw none, parse_token_type_end} [x|
| ]
{off    7, len    0, indent  0, kw none, symbol_job_list} [\cJ|| ]
echo x
Member

floam commented Jul 2, 2016

When would one use this instead of the __fish_parse builtin? (it sure is prettier and doing roughly the same thing... maybe the goals are different.)

> echo "echo x" | __fish_parse

 0 -  0  job_list <2 children>  [0, 7]
 1 -  1    job <3 children>  [0, 6]
 2 -  3      statement <1 children>  [0, 6]
 3 -  6        decorated_statement <1 children>  [0, 6]
 4 -  7          plain_statement <2 children>  [0, 6]
 5 -  8            token_string: "echo"
 6 -  9          arguments_or_redirections_list <2 children>  [5, 1]
 7 - 10            argument_or_redirection <1 children>  [5, 1]
 8 - 12              symbol_argument <1 children>  [5, 1]
 9 - 13                token_string: "x"
10 - 11          arguments_or_redirections_list  [no src]
11 -  4      job_continuation  [no src]
12 -  5      optional_background  [no src]
13 -  2  job_list <2 children>  [6, 1]
14 - 14    token_end  [6, 1]
15 - 15  job_list  [no src]

> echo "echo x" | fish_indent -d

{off    0, len    7, indent  0, kw none, symbol_job_list} [ |echo x
| ]
{off    0, len    6, indent  0, kw none, symbol_job} [ |echo x|\cJ]
{off    0, len    6, indent  0, kw none, symbol_statement} [ |echo x|\cJ]
{off    0, len    6, indent  0, kw none, symbol_decorated_statement} [ |echo x|\cJ]
{off    0, len    6, indent  0, kw none, symbol_plain_statement} [ |echo x|\cJ]
{off    0, len    4, indent  0, kw none, parse_token_type_string} [ |echo| ]
{off    5, len    1, indent  0, kw none, symbol_arguments_or_redirections_list} [ |x|\cJ]
{off    5, len    1, indent  0, kw none, symbol_argument_or_redirection} [ |x|\cJ]
{off    5, len    1, indent  0, kw none, symbol_argument} [ |x|\cJ]
{off    5, len    1, indent  0, kw none, parse_token_type_string} [ |x|\cJ]
{off    6, len    0, indent  0, kw none, symbol_arguments_or_redirections_list} [x||\cJ]
{off    6, len    0, indent  0, kw none, symbol_job_continuation} [x||\cJ]
{off    6, len    0, indent  0, kw none, symbol_optional_background} [x||\cJ]
{off    6, len    1, indent  0, kw none, symbol_job_list} [x|
| ]
{off    6, len    1, indent  0, kw none, parse_token_type_end} [x|
| ]
{off    7, len    0, indent  0, kw none, symbol_job_list} [\cJ|| ]
echo x
@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jul 4, 2016

Contributor

When would one use this instead of the __fish_parse builtin? (it sure is prettier and doing roughly the same thing... maybe the goals are different.)

I looked at the __fish_parse builtin and for whatever reason it didn't give me the insight I needed to implement the enhancements to fish_indent I was working on. It might be, and probably is, a good idea to merge the two implementations. But that's outside the scope of this issue and is thus issue for another day.

Contributor

krader1961 commented Jul 4, 2016

When would one use this instead of the __fish_parse builtin? (it sure is prettier and doing roughly the same thing... maybe the goals are different.)

I looked at the __fish_parse builtin and for whatever reason it didn't give me the insight I needed to implement the enhancements to fish_indent I was working on. It might be, and probably is, a good idea to merge the two implementations. But that's outside the scope of this issue and is thus issue for another day.

@krader1961 krader1961 closed this in 3c4e322 Jul 6, 2016

@floam floam modified the milestones: next-2.x, fish-future Aug 2, 2016

@krader1961 krader1961 modified the milestones: fish 2.3.2, next-2.x Sep 3, 2016

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