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 readability #1571

Merged
merged 2 commits into from
Mar 7, 2021
Merged

Improve readability #1571

merged 2 commits into from
Mar 7, 2021

Conversation

matklad
Copy link
Contributor

@matklad matklad commented Mar 2, 2021

Using Paths for paths expresses intent more clearly.

Drive by change mostly, I just accidentally fell into non-utf8 arguments pit :)

@@ -341,7 +341,7 @@ mod tests {
fn syntax_for_file_with_content_os(&self, file_name: &OsStr, first_line: &str) -> String {
let file_path = self.temp_dir.path().join(file_name);
let input = Input::from_reader(Box::new(BufReader::new(first_line.as_bytes())))
.with_name(Some(file_path.as_os_str()));
.with_name(Some(&file_path));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that here we don't need a conversion back from Path.

.matches
.values_of("file-name")
.map(|values| values.collect());
.values_of_os("file-name")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This adds support for non-utf8 names

@Enselic
Copy link
Collaborator

Enselic commented Mar 2, 2021

I haven't done a detailed review yet, but am in general positive to this change after skimming it through.

I'm curious what problem you saw with non-UTF-8 file names though? Works OK for me at least, using official build of bat 0.18.0 on x86_64-unknown-linux-gnu:

% echo testing > $(printf "\x$(printf %x 130)")       
% bat $(printf "\x$(printf %x 130)") 
───────┬────────────────────────────────
       │ File: �
───────┼────────────────────────────────
   1   │ testing
───────┴────────────────────────────────
% bat --version                                                 
bat 0.18.0

@matklad
Copy link
Contributor Author

matklad commented Mar 2, 2021

@Enselic you need to check with the file-names argument

@sharkdp
Copy link
Owner

sharkdp commented Mar 2, 2021

Thank you for your contribution!

The changes look good. This is (at least technically) a breaking change, so it should be documented as such in the "bat as a library" section of the CHANGELOG.md. Changing AsRef<OsStr> to AsRef<Path> is unlikely to cause any problems (I guess), but the changes for Input::ordinary_file and Input::with_name could lead to errors in client code. Maybe we could accept AsRef<Path> for these methods as well? That would still allow users to pass in OsStrs, I believe.

@Enselic
Copy link
Collaborator

Enselic commented Mar 3, 2021

Thanks, with --file-names I can confirm that it crashes:

% bat --file-names $(printf "\x$(printf %x 130)")      
thread 'main' panicked at 'unexpected invalid UTF-8 code point', /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.33.3/src/app/parser.rs:1683:33
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Using `Path`s for paths expresses intent more clearly.
@matklad
Copy link
Contributor Author

matklad commented Mar 3, 2021

Yeah, makes sense, updated!

@sharkdp sharkdp merged commit 2e7f2b6 into sharkdp:master Mar 7, 2021
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.

3 participants