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

Can read files named "cache" now, cache subcommand working too #275

Merged
merged 18 commits into from
Sep 5, 2018

Conversation

BK1603
Copy link
Contributor

@BK1603 BK1603 commented Sep 4, 2018

Now if a cache file exists in the current directory, and the user passes
no arguments to the cache command, the cache file would be displayed.
If however the user uses cache command with arguments, the cache command
would be executed as normal regardless of wether the file cache exists
in the current directory or not.

Though now there won't be an error message displayed if the user uses the cache sub command without arguments in any directory that contains a file named cache.

Now if a cache file exists in the current directory, and the use passes
no arguments to the cache command, the cache file would be displayed.
If however the user uses cache command with arguments, the cache command
would be executed as normal regardless of wether the file cache exists
in the current directory or not.
@BK1603
Copy link
Contributor Author

BK1603 commented Sep 5, 2018

Okay so like I have tried everything I could to pass the travis check, but I just can not pass it 😅

Like, please look at it and let me know what's wrong, cargo fmt says things have been changed but then removes and adds the same line, I am pretty confused.

@BK1603 BK1603 changed the title Fixed issue #245 Can read files named "cache" now, cache subcommand working too Sep 5, 2018
@sharkdp
Copy link
Owner

sharkdp commented Sep 5, 2018

Thank you very much for your contribution!

Like, please look at it and let me know what's wrong, cargo fmt says things have been changed but then removes and adds the same line, I am pretty confused.

It's just trailing whitespace that rustfmt is complaining about. I fixed it. For the future: you can just use cargo fmt and it will just auto-format all the code for you 😄.

run_cache_subcommand(cache_matches)?;
Ok(true)
} else {
let mut config = app.config()?;
Copy link
Owner

Choose a reason for hiding this comment

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

The code duplication in this else-clause is kind of unfortunate (it mirrors most steps that we normally do, if no subcommand is given).

I was wondering whether we could simply take out the cache subcommand completely if a cache file exists. We wouldn't get the behavior which you implemented here (i.e. that bat cache --init still works)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Umm, yeah I thought about it, like implementing that in the app.rs file, but, umm, how about we make a function for calling the controller and call it first here and then in the other match clause?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or else we can do what you are suggesting, but yep that is gonna drop the functionality there 😅

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, sounds good. We should probably move the HighlightingAssets::new() part into this new function (and remove the assets argument from list_languages and list_themes and just call HighlightingAssets::new() inside those functions).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will be doing what you just said! Should be ready in a few minutes!

src/app.rs Outdated
@@ -102,6 +103,10 @@ impl App {
AppSettings::ColorNever
};

// Check if the current directory contains a file name cache, if it does
// do not make the arguements for subcommand 'cache' required.
let arg_group_required = !Path::new("./cache").exists();
Copy link
Owner

Choose a reason for hiding this comment

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

I think this can be just "cache", right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeagh 😓
Just a minute let me fix that

@BK1603
Copy link
Contributor Author

BK1603 commented Sep 5, 2018

Sorry for all the mess 😅

Also if there is anything else that isn't like, with the conventions please let me know

And thanks a lot for reviewing the code and telling me about the mistakes 😄

@sharkdp
Copy link
Owner

sharkdp commented Sep 5, 2018

Do you want me to call cargo fmt again? 😄

@BK1603
Copy link
Contributor Author

BK1603 commented Sep 5, 2018

I fixed it 😅
Like when I ran cargo fmt on my laptop, it added a few new lines that travis didn't seem to like 😅
I apologize once again 😓
And thanks a lot once again for reviewing the code 😄

@sharkdp
Copy link
Owner

sharkdp commented Sep 5, 2018

Like when I ran cargo fmt on my laptop, it added a few new lines that travis didn't seem to like
I apologize once again

No worries! You need to make sure to have the latest version of rustfmt-nightly. The style changes slightly from time to time.

And thanks a lot once again for reviewing the code

Well, thank you for your contribution!

@BK1603
Copy link
Contributor Author

BK1603 commented Sep 5, 2018

IT'S DONE!

@sharkdp sharkdp merged commit 53d0c1d into sharkdp:master Sep 5, 2018
@sharkdp
Copy link
Owner

sharkdp commented Sep 12, 2018

Released in v0.7.0.

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