Issue 2 check ownership groups #7
Conversation
…ix OS's.
Maybe the API usage looks like:
```rs
fn main() {
let path = Path::new("...");
if path.is_permitted()?
.is_executable() {
// ...
}
profit!();
}
```
and ideally initiate a division of concerns between Linux-y/Unix-y permission bit flags,
as well as whatever Windows has in store.
…c in the documentation. At that's left to do is clean up the type signatures, fat pointers, and borrows.
…t implements Deref<Path>.
|
For #2 |
|
@fitzgen Would you be able to offer any guidance or suggestions on what I need to do to make this pass? This is my first open source PR, so I'm a little lost. |
There was a problem hiding this comment.
Hey @dmgolembiowski - I'm not a maintainer or the owner of this repo but I stumbled across your PR and thought it deserved a look at since you're trying to give back to the community!
Other general notes that don't really fit anywhere:
- Your commit history is messing. Look into using "git interactive rebase" if you're unfamiliar with it and squash your work down into small, atomic commits, that each work by themselves. In my PR (Try and determine executability using file extension on Windows #6), I made one commit for the functionality change, another to add the tests. Something similar would probably make sense here. You'll need to force push your branch to amend things in this PR
- If you use the phrase "Will close" in your original PR message followed by the issue number, GitHub will automagically identify that your PR will close an issue if it's merged, saved the owner or a maintainer having to do that manually
Please note I'm running Windows so I'm not actually able to test your additions, just can help you make your code compile, pass tests, and get this PR tidy ready for reviewing to be merged :)
Also, this is my first time ever code reviewing, so I hope this was helpful!
| @@ -18,3 +18,6 @@ winapi = { version = "0.3", features = ["winbase"] } | |||
|
|
|||
| [dev-dependencies] | |||
| diff = "0.1.10" | |||
|
|
|||
| [dependencies] | |||
There was a problem hiding this comment.
This dependency should be Unix only, see this section of the Cargo book for a how to
| [dependencies] | |
| [target.'cfg(unix)'.dependencies] |
| /// | ||
| /// See the module documentation for details. | ||
| #[cfg(unix)] | ||
| pub fn is_permitted<P>(path: P) -> Result<::std::path::PathBuf, io::Error> |
There was a problem hiding this comment.
Use the full path for io here and get rid of the import on line 58, it's unneeded unless compiling for Unix
| @@ -68,6 +71,8 @@ where | |||
| path.as_ref().is_executable() | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Adding random whitespace isn't good practice for creating a PR. You don't want to change anything more than necessary
| /// In other words, we'll collect each of the (G*) entries | ||
| /// and see if there's a match. Otherwise, we check who owns the file and | ||
| /// perform a similar check. | ||
| impl IsPermitted for Path { |
There was a problem hiding this comment.
The formatting within this impl is questionable. I'd recommend trying to use rustfmt if you can do without massively changing other parts of the lib, or tidying this up yourself by hand
| @@ -1,7 +1,7 @@ | |||
| extern crate diff; | |||
| extern crate is_executable; | |||
|
|
|||
| use is_executable::is_executable; | |||
| use is_executable::{ is_executable, is_permitted }; | |||
There was a problem hiding this comment.
This import won't work when compiling for not-Unix. Importing is_permitted must be done in mod unix
| /// | ||
| /// See the module documentation for examples. | ||
| #[cfg(unix)] | ||
| pub trait IsPermitted { |
There was a problem hiding this comment.
Both this trait declaration and is_permitted might as well go into the mod unix, You may need to add a pub use statement to re-export what's needed to the base of the crate if that's what you're after
Added a feature extension to check the group and owner bits on the file in question. The feature is available for Unix OS's.