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

Allow 'bat' application to built without git feature, remove PrettyPrint git support when feature not enabled. #997

Merged
merged 3 commits into from
May 15, 2020

Conversation

eth-p
Copy link
Collaborator

@eth-p eth-p commented May 13, 2020

This pull request changes two things:

  1. (bat-as-a-library; breaking change)
    When the git feature is not enabled, it removes PrettyPrinter::vcs_modification_markers. This function was previously being exposed when the feature was disabled, despite not having any effect when used.

  2. (bat)
    Adds cfg flags to allow the bat application to build without requiring the git feature. This would allow people to build bat without git support if they don't feel they would need or use it.
    Caveats: Unfortunately, disabling the feature doesn't change the help text or generated manpage.

@eth-p eth-p requested a review from sharkdp May 13, 2020 07:46
@sharkdp
Copy link
Owner

sharkdp commented May 15, 2020

Adds cfg flags to allow the bat application to build without requiring the git feature. This would allow people to build bat without git support if they don't feel they would need or use it.

Thank you, looks good to me!

When the git feature is not enabled, it removes PrettyPrinter::vcs_modification_markers. This function was previously being exposed when the feature was disabled, despite not having any effect when used.

Okay.

Caveats: Unfortunately, disabling the feature doesn't change the help text or generated manpage.

I guess that's something we can live with. Help text (--help) should be changed, right?

I have two comments:

  1. The match … { true => …, … } part looks weird and is hard to read, in my opinion. I will add an inline comment
  2. Could you please adapt the CHANGELOG.md accordingly? I am now trying to always keep this up to date in order to facilitate the release process. I guess we should add a PR template / CONTRIBUTING.md file to explain this to all contributors.

For now, I have set up a "saved reply" along the lines of:

Could you please add an entry to the "unreleased" section in the CHANGELOG.md? Under "Features" or "Other". The format for entries is:

- Description what has been changed, see #123 (@user)

where #123 links to the bug ticket and/or this PR and user is your username.

Comment on lines +201 to +203
visible_lines: match self.matches.is_present("diff") {
#[cfg(feature = "git")]
true => VisibleLines::DiffContext(
Copy link
Owner

Choose a reason for hiding this comment

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

I see why you chose to use a match statement here, but this is a bit difficult to read. I don't know a better solution of the top of my head, though.

I guess if cfg!(…) does not work because DiffContext is not available if git is not set?

Copy link
Collaborator Author

@eth-p eth-p May 15, 2020

Choose a reason for hiding this comment

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

I guess if cfg!(…) does not work because DiffContext is not available if git is not set?

Pretty much. I agree that it's a bit difficult to read, but it was also the smallest change to make it work properly without duplicating code to handle both git support and no git support.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok

@eth-p
Copy link
Collaborator Author

eth-p commented May 15, 2020

I guess that's something we can live with. Help text (--help) should be changed, right?

Sort of. It removes the related entries, but it doesn't remove any references to them from the descriptions of other arguments (e.g. the description for --style). I wasn't sure of how we could do that without writing multiple different help texts and #[cfg]ing each of them, and I'm not a fan of doing that because of the maintenance overhead and potential to forget to update one of the different description variants.

Could you please adapt the CHANGELOG.md accordingly? I am now trying to always keep this up to date in order to facilitate the release process. I guess we should add a PR template / CONTRIBUTING.md file to explain this to all contributors.

For future reference, does this include testing or developer-only (internal, not bat-as-a-library) changes?

@sharkdp
Copy link
Owner

sharkdp commented May 15, 2020

I guess that's something we can live with. Help text (--help) should be changed, right?

Sort of. It removes the related entries, but it doesn't remove any references to them from the descriptions of other arguments (e.g. the description for --style). I wasn't sure of how we could do that without writing multiple different help texts and #[cfg]ing each of them, and I'm not a fan of doing that because of the maintenance overhead and potential to forget to update one of the different description variants.

I agree.

For future reference, does this include testing or developer-only (internal, not bat-as-a-library) changes?

Good question. I would exclude everything that is not of interest to (1) users of bat or (2) users of bat as a library.

@eth-p
Copy link
Collaborator Author

eth-p commented May 15, 2020

Rebased and added a changelog update. I didn't mention building bat without support for the git feature since it's not relevant for the majority of bat users, and would require them to manually compile the binary.

@eth-p eth-p merged commit 9c371ed into sharkdp:master May 15, 2020
@sharkdp
Copy link
Owner

sharkdp commented May 25, 2020

Released in bat v0.15.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants