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

Show Inaccuracy Warning on permission error #270

Merged
merged 8 commits into from Nov 9, 2018

Conversation

2 participants
@Veykril
Contributor

Veykril commented Oct 1, 2018

This PR roughly implements issue #165. I am sure there is room for some improvements on my code changes so feel free to comment on everything I changed. One thing I dislike that I did is this weird if let bundle I created to check for a permission error at one place:
https://github.com/Veykril/tokei/blob/0c7e18d1b0b45832c40c3a991be4bd57111ff5e5/src/utils/fs.rs#L52-L61
I couldnt come up with a better way since the nested errors are boxed and box patterns arent stable yet.

Example Output:

C:\Workspace\Rust\tokei (error_output -> origin)                                                                       
λ cargo run -- C:\Workspace\Rust\test                                                                                  
    Finished dev [unoptimized + debuginfo] target(s) in 0.18s                                                          
     Running `target\debug\tokei.exe C:\Workspace\Rust\test`                                                           
ERROR 2018-10-01T16:28:56Z: tokei::utils::fs: Permission denied for reading C:\Workspace\Rust\test\Cargo.toml          
Note: results can be inaccurate for languages marked with '(!)'                                                        
-----------------------------------------------------------------------------------------------------------------------
 Language                                                    Files        Lines         Code     Comments       Blanks 
-----------------------------------------------------------------------------------------------------------------------
 Markdown                                                        4         1380         1380            0            0 
 Rust                                                            1          123           99            1           23 
 Shell                                                           1           40           29            1           10 
 TOML (!)                                                        1           84           69            0           15 
 YAML                                                            1           80           60            8           12 
-----------------------------------------------------------------------------------------------------------------------
 Total (!)                                                        8         1707         1637           10           60
-----------------------------------------------------------------------------------------------------------------------
@Veykril

This comment has been minimized.

Contributor

Veykril commented Oct 5, 2018

@Aaronepower sorry for pinging you but I wasnt sure whether you didn't see this PR or not

@Aaronepower

This comment has been minimized.

Owner

Aaronepower commented Oct 6, 2018

@Veykril Sorry, yes I did see it. I have a problem where if I'm unsure about a feature I tend to not reply till I have a more definitive answer 😅. Could you run benchmark.sh in the root of the repo and post the output here?

@Veykril

This comment has been minimized.

Contributor

Veykril commented Oct 6, 2018

I was concerned about the performance impact of this as well but i couldn't think of a different way to implement this.

Benchmark 1: tokei "C:WorkspaceRusttokei"  
Summary
Time (mean ± σ):      13.1 ms ±   2.1 ms    [User: 5.3 ms, System: 4.3 ms]
Range (min … max):    10.1 ms …  22.9 ms

Benchmark 2: tokei "C:WorkspaceRusttokei"
Time (mean ± σ):      14.2 ms ±   2.9 ms    [User: 4.2 ms, System: 5.5 ms]
Range (min … max):    10.0 ms …  26.0 ms
Summary
  'tokei.exe "C:WorkspaceRusttokei"' ran
    1.08 ± 0.28 times faster than 'tokei "C:WorkspaceRusttokei"'

I don't think these are accurate though, my machine isnt the best to benchmark things + Im running windows so im not too sure how well that goes.

@Aaronepower

This comment has been minimized.

Owner

Aaronepower commented Oct 7, 2018

@Veykril Sorry, could you run the benchmark on a larger repo such as the rust repo? Also could you have two benchmarks, one with no warnings and one with a warning?

@Veykril

This comment has been minimized.

Contributor

Veykril commented Oct 7, 2018

No problem, i can see your concern over this since your tool should stay as fast as possible.

I ran it on the rust repo now and I moved and renamed my exe out of the build dir cause i wasnt sure how windows handles path exes and the exes in the current dir, so iwanted to be safe that it actually executes my build.

Output without an error: https://pastebin.com/3xHqpUJT
Output with an error: https://pastebin.com/ALFnSJqx

@Aaronepower

This comment has been minimized.

Owner

Aaronepower commented Oct 15, 2018

@Veykril Sorry about this, I've since made a large scale refactor to the codebase that makes the changes here incompatible. You'll need to rewrite some of this for it to be accepted.

@Veykril

This comment has been minimized.

Contributor

Veykril commented Oct 16, 2018

No problem, will look into your changes sometime later this week and try to adapt my PR to that

@Veykril

This comment has been minimized.

Contributor

Veykril commented Oct 18, 2018

Alright, I adapted the changes and tested them. From what I have seen I didnt have to change much unless I've overlooked something. I also fixed an output bug that I didnt even notice at first, as you can see in the example output text from the PR, the total section wasnt correctly aligned. That shouldnt happen anymore now.

@@ -104,6 +112,9 @@ impl AddAssign for Language {
self.blanks += rhs.blanks;
self.code += rhs.code;
self.stats.extend(mem::replace(&mut rhs.stats, Vec::new()));
if rhs.inaccurate {
self.inaccurate = rhs.inaccurate

This comment has been minimized.

@Aaronepower

Aaronepower Oct 19, 2018

Owner

Can you remove the if statement and replace with the following?

Suggested change Beta
self.inaccurate = rhs.inaccurate
self.inaccurate |= rhs.inaccurate
/// Parses the text provided.
pub fn parse_from_str_with_accuracy(self, path: PathBuf, text: &str) -> Stats

This comment has been minimized.

@Aaronepower

Aaronepower Oct 19, 2018

Owner

Why is parse_from_str_with_accuracy necessary?

This comment has been minimized.

@Veykril

Veykril Oct 19, 2018

Contributor

oh it actually isnt now that you say it, i was just confused as to why parse_from_strreturns a result when it never returns an error. Probably due to previous implementations I guess? Will remove the function again

use ignore::Error;
if let Error::WithDepth { err: ref error, .. } = error {
if let Error::WithPath { ref path, err: ref error } = **error {
if let Error::Io(ref error) = **error {

This comment has been minimized.

@Aaronepower

Aaronepower Oct 20, 2018

Owner

I think we can condense this code. Instead of just checking for Permission denied, couldn't we use the following?

error!("{} reading {}", error.description(), path.display());

This comment has been minimized.

@Veykril

Veykril Oct 20, 2018

Contributor

Yep, this is indeed better, I guess I was too focused on the PermissionDenied error there. Looks a lot better like this and makes more sense overall

@Aaronepower

I have a few last change requests 😅 Thank you for your PR and your patience through this process.

Show resolved Hide resolved src/language/language_type.rs
s
};
self.parse_from_str(path, &text)
Ok(self.parse_from_str(path, &text).unwrap())

This comment has been minimized.

@Aaronepower

Aaronepower Oct 20, 2018

Owner

Can you remove the Ok() and the unwrap()?

This comment has been minimized.

@Veykril

Veykril Oct 20, 2018

Contributor

Oh right, I did this cause the results have different error cases and parse_from_str cannot fail from its implementation. Probably a bad idea to unwrap though since the implementation could change. Will have to do the same if let Err instead of map_err though due to the closure move that would happen otherwise though

This comment has been minimized.

@Veykril

Veykril Oct 20, 2018

Contributor

Actually i have a problem here in general cause the parse_from_str error that wouldnt happen in its current implementation doesnt give back the pathbuf, so I am not sure how to even map the error here

}
/// Parses the text provided. Returning `Stats` on success.
pub fn parse_from_str(self, path: PathBuf, text: &str)
-> io::Result<Stats>
pub fn parse_from_str(self, path: PathBuf, text: &str) -> io::Result<Stats>

This comment has been minimized.

@Aaronepower

Aaronepower Oct 20, 2018

Owner

Can you revert the style changes on this method? I prefer the style used previously.

Show resolved Hide resolved src/language/language_type.rs
@Veykril

This comment has been minimized.

Contributor

Veykril commented Oct 20, 2018

No problem, I am still learning you pointing all these things out is only helping me in the end. Better to do this properly than just letting everything through 👍

@Aaronepower

This comment has been minimized.

Owner

Aaronepower commented Oct 21, 2018

@Veykril Okay, I think if you could make the style change and resolve the conflicts this is ready to merge in.

@Aaronepower

This comment has been minimized.

Owner

Aaronepower commented Oct 21, 2018

Haha, sorry you actually can break the API as I'm going to be making some changes that would also break the API so we might as well break them both in the same release.

@Veykril

This comment has been minimized.

Contributor

Veykril commented Oct 22, 2018

So i can change the function signatures as I please?

@Aaronepower

This comment has been minimized.

Owner

Aaronepower commented Oct 23, 2018

@Veykril Remove the accuracy functions and change to the API of the current ones.

@Veykril

This comment has been minimized.

Contributor

Veykril commented Oct 25, 2018

I suppose this is what you meant right? I hope

@Aaronepower

This comment has been minimized.

Owner

Aaronepower commented Nov 9, 2018

Okay, this PR looks good to me. Thank you again for your PR and for making the requested changes!

@Aaronepower Aaronepower merged commit 51ecef4 into Aaronepower:master Nov 9, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment