Skip to content

Add options to disable#43

Merged
sharkdp merged 3 commits intomasterfrom
unknown repository
May 3, 2018
Merged

Add options to disable#43
sharkdp merged 3 commits intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented May 2, 2018

  • line number
  • git modification marker
  • file name

fix #5

Add options to disable
3939de4 
* line number
* git modification marker
* file name

fix #5
sharkdp
sharkdp requested changes May 2, 2018
View reviewed changes
Copy link
Copy Markdown
Owner

@sharkdp sharkdp left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome and thank you very much for working on this!

The implementation looks good but I would really like to avoid having to add three command line arguments for this.

Could we please discuss some alternatives first?

For example: a --style with several arguments (plain, numbers, ..., full)?

Do we need the file titles anyway? Does this need to be configurable? Maybe we should just show them if there is more than one file and disable them by default?

src/main.rs Outdated
.arg(
Arg::with_name("disable-git-modfication-marker")
.short("g")
.long("disable-git-modfication-marker")
Copy link
Copy Markdown
Owner

@sharkdp sharkdp May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: modfication => modification. Maybe just disable-git-markers?

src/main.rs Outdated
.arg(
Arg::with_name("disable-line-number")
.short("n")
.long("disable-line-number")
Copy link
Copy Markdown
Owner

@sharkdp sharkdp May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe disable-line-numbers?

src/main.rs Outdated
.arg(
Arg::with_name("disable-file-name")
.short("f")
.long("disable-file-name")
Copy link
Copy Markdown
Owner

@sharkdp sharkdp May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe disable-file-names?

@ghost
Copy link
Copy Markdown
Author

ghost commented May 3, 2018 •
edited by ghost
Loading

Thanks for such a quick feedback. Sure I would say

  • We can have a single clap arg called style like as you suggested with below possible values
    • plain/minimal - only content
    • numbers - only numbers
    • full - numbers, and markers
  • Another alternative would be, we can have arguments for line number - -n, --number (same as cat) and -g, --git-modification-marker and drop the ones for lines. I used singular longer names as I found the same in cat, grep, etc.

In addition

  • We can change filename to only show if multiple files
  • We can show the horizontal lines only in case of multiple files

What do you think? I do not have any strong opinions, except that the arguments which default tools have should match so one can easily switch from default to rust alternatives for basic programs.

@sharkdp
Copy link
Copy Markdown
Owner

sharkdp commented May 3, 2018

We can have a single clap arg like as you suggested with below possible values
plain/minimal - only content
numbers - only numbers
full - numbers, and markers

Sounds great.

We can change filename to only show if multiple files

I thouhgt about this again.. the header bar with the file name is not super-helpful for a single file right now, but I was planning to add some more information to that line (git status, number of added/deleted/modified lines, last modified by, etc.). We should definitely disable the header for --style=plain, but I think I'd like to keep it enabled by default, for now.

In addition we can show the horizontal lines only in case of multiple files

If we keep the header, I'd also like to keep the horizontal lines for now. Again, they should definitely be removed for --style=plain. Maybe we could later add some --grid option that would allow us to set the style for all grid lines (off, only vertical lines, plain ASCII characters instead of Unicode block-drawing symbols, ...).

Thoughts?

@ghost
Copy link
Copy Markdown
Author

ghost commented May 3, 2018

Sure. Then lets do the style option with

  • plain - no line numbers and no modification markers
  • line-numbers - only line numbers
  • full - line numbers and markers for now, and in future other things

Nakul Chaudhari added 2 commits May 3, 2018 18:14
Add simple style option to enable/disable additional info
ada0b6a 
Remove previously added options to disable
* line number
* git modification marker
* file name

fix #5
Merge branch 'master' of https://github.com/sharkdp/bat
8e7c934
@sharkdp sharkdp merged commit 7df9a5f into sharkdp:master May 3, 2018
@sharkdp
Copy link
Copy Markdown
Owner

sharkdp commented May 3, 2018

Thank you!

@sharkdp
Copy link
Copy Markdown
Owner

sharkdp commented May 3, 2018

Oh, for some reason I thought it would completely disable the horizontal/vertical bars for --style=plain.

@ghost
Copy link
Copy Markdown
Author

ghost commented May 3, 2018

Ah, yes, now I see that you did mention it. But somehow I confused it and retained it as we wanted to retain the name anyway and with multiple files this would anyway be an issue.

@sharkdp
Copy link
Copy Markdown
Owner

sharkdp commented May 3, 2018

and retained it as we wanted to retain the name anyway

I think "plain" should really just be the file contents, nothing else. Sorry if I confused you with the file-name/header thing.

and with multiple files this would anyway be an issue.

Why? It would be the same as the normal cat behavior. The content of the different files would be concatenated.

Would you agree with these statements? If yes, would you like to address this in another PR? Otherwise, I can also implement this myself.

In any case, thanks!

@ghost
Copy link
Copy Markdown
Author

ghost commented May 3, 2018

Please see the latest PR

@sharkdp sharkdp mentioned this pull request May 6, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add option to disable line numbers

1 participant

@sharkdp