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

Improve Manpage syntax #1315

Merged
merged 4 commits into from
Oct 17, 2020
Merged

Conversation

keith-hall
Copy link
Collaborator

@keith-hall keith-hall commented Oct 14, 2020

Fixes #1148
(and also relates to #1213 )

How git push --help looks after these changes when using bat as the pager:

image

@sharkdp
Copy link
Owner

sharkdp commented Oct 14, 2020

Thank you very much! The new man page for git push looks great 👍

Unfortunately, this seems to break C syntax highlighting for man pages like select(2) which look like this, currently:

image

I hope it's fine that I push a select(2) syntax regression test that should fail on this branch. We don't have to keep this exact style (meaning: it's fine if we have to push a new highlighted/Manpage/select-2.man version) but it would be great if the C syntax highlighting still works.

@sharkdp
Copy link
Owner

sharkdp commented Oct 14, 2020

It seems like I can not push to your fork, but you can cherry-pick e1af838 if you want.

@keith-hall
Copy link
Collaborator Author

Well spotted, thanks! It is definitely useful to have these regression tests 👍 I have cherry picked your commit and pushed some fixes. It uses some "heuristics" to determine if it is C code or not - namely presence of a block comment or #include directive - if you know of any man pages where this won't work, please let me know!

(about not being able to push to my fork: sorry about that. I like to keep forks separate from my own repos, so I created a GitHub org for them, but unfortunately GitHub since stopped allowing maintainers push access - it doesn't even show the option when creating the PR. I guess it makes sense because organizations have their own permissions/rules)

@sharkdp
Copy link
Owner

sharkdp commented Oct 17, 2020

Well spotted, thanks! It is definitely useful to have these regression tests +1 I have cherry picked your commit and pushed some fixes. It uses some "heuristics" to determine if it is C code or not - namely presence of a block comment or #include directive - if you know of any man pages where this won't work, please let me know!

That sounds great 👍

(about not being able to push to my fork: sorry about that. I like to keep forks separate from my own repos, so I created a GitHub org for them, but unfortunately GitHub since stopped allowing maintainers push access - it doesn't even show the option when creating the PR. I guess it makes sense because organizations have their own permissions/rules)

No worries. You can also create branches in this repo, if you prefer that.

@sharkdp sharkdp merged commit 5d9adc1 into sharkdp:master Oct 17, 2020
@sharkdp
Copy link
Owner

sharkdp commented Oct 17, 2020

Thank you very much for the improvements and the detailed tests!

@sharkdp
Copy link
Owner

sharkdp commented Oct 17, 2020

There's actually a large C code block at the bottom of select(2) which is not highlighted (was not highlighted before either):

EXAMPLES
       #include <stdio.h>
       #include <stdlib.h>
       #include <sys/select.h>

       int
       main(void)
       {
           fd_set rfds;
           struct timeval tv;
           int retval;

           /* Watch stdin (fd 0) to see when it has input. */

           FD_ZERO(&rfds);
           FD_SET(0, &rfds);
…

Should that have been highlighted by your change? If not, that's also fine.

@keith-hall
Copy link
Collaborator Author

It shouldn't have been - I didn't notice it and didn't change under which sections it applies. I have now pushed a new commit to the manpage-improvements branch addressing this.
There are a few changes in the regression test which I wasn't expecting, however, like
e0b7b80#diff-c601da59bcdb7dd775083ccc40783338d94e9d2ed4fefc316b698189e654addfL149

@sharkdp
Copy link
Owner

sharkdp commented Oct 17, 2020

There is a "visual diff" in the GA output: https://github.com/sharkdp/bat/runs/1268838933?check_suite_focus=true

Can't really tell why these changes happen though.

I guess the manpage syntax will always stay on a "best effort" basis, as there is no well-defined "syntax", to my knowledge. So I guess I'm fine with most changes (even if there are a few regressions) as long as there is an overall improvement.

@keith-hall keith-hall mentioned this pull request Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

man: Optional parts of arguments are not hightlighting
2 participants