Skip to content

Commit

Permalink
Use OptionExt::contains instead of local option_contains function
Browse files Browse the repository at this point in the history
  • Loading branch information
soc committed Apr 11, 2023
1 parent 5ac52ce commit e169da7
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 9 deletions.
3 changes: 3 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ license = "MIT OR Apache-2.0"
repository = "https://github.com/dirs-dev/dirs-sys-rs"
maintenance = { status = "as-is" }

[dependencies]
option-ext = "0.2.0"

[target.'cfg(unix)'.dependencies]
libc = "0.2"

Expand Down
2 changes: 2 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
extern crate option_ext;

use std::ffi::OsString;
use std::path::PathBuf;

Expand Down
14 changes: 5 additions & 9 deletions src/xdg_user_dirs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ use std::os::unix::ffi::OsStringExt;
use std::path::{Path, PathBuf};
use std::str;

use option_ext::OptionExt;

/// Returns all XDG user directories obtained from $(XDG_CONFIG_HOME)/user-dirs.dirs.
pub fn all(home_dir_path: &Path, user_dir_file_path: &Path) -> HashMap<String, PathBuf> {
let bytes = read_all(user_dir_file_path).unwrap_or(Vec::new());
Expand All @@ -32,7 +34,7 @@ fn parse_user_dirs(home_dir: &Path, user_dir: Option<&str>, bytes: &[u8]) -> Has
let key = if key.starts_with(b"XDG_") && key.ends_with(b"_DIR") {
match str::from_utf8(&key[4..key.len()-4]) {
Ok(key) =>
if user_dir.is_some() && option_contains(user_dir, key) {
if user_dir.contains(&key) {

This comment has been minimized.

Copy link
@nickelc

nickelc Apr 15, 2023

Contributor

Why not just use if user_dir == Some(key) here?

This comment has been minimized.

Copy link
@soc

soc Apr 15, 2023

Author Collaborator

I feel that contains documents the intent slightly better.

This comment has been minimized.

Copy link
@fujiapple852

fujiapple852 May 1, 2023

Landed here as my cargo deny objected to the licence of the newly introduced crate.

rejected: license is considered copyleft

I’m no OSS licence expert, I see some say MPL-2.0 is compatible with Apache 2 / MIT but cargo deny doesn’t seem to agree, which makes dirs unsuitable for some projects.

It would be useful if you could duel licence option-ext, is that something you’d consider @soc?

Aside, could the above not simply be an if let statement and avoid the need for an extra dependency?

This comment has been minimized.

Copy link
@soc

soc May 2, 2023

Author Collaborator

See #21.

This comment has been minimized.

Copy link
@daira

daira Jun 26, 2023

I cannot see how if user_dir == Some(key) is less clear at all; it expresses precisely what is meant. On the contrary, to understand if user_dir.contains(&key) you need to know that the option-ext crate provides a extension trait for Option that is used in this line of code. Even knowing that, it took me 20 seconds to look up the documentation and be sure that the usage is correct, vs a fraction of a second to understand what if user_dir == Some(key) does (not to mention more than an hour I've spent dealing with this licensing issue so far!)

if user_dir == Some(key) is the approach I've used in #24.

single_dir_found = true;
key
} else if user_dir.is_none() {
Expand Down Expand Up @@ -137,18 +139,12 @@ fn shell_unescape(escaped: &[u8]) -> Vec<u8> {
unescaped
}

fn option_contains<T : PartialEq>(option: Option<T>, value: T) -> bool {
match option {
Some(val) => val == value,
None => false
}
}

#[cfg(test)]
mod tests {
use std::collections::HashMap;
use std::path::{Path, PathBuf};
use super::{trim_blank, shell_unescape, split_once, parse_user_dirs};

use super::{parse_user_dirs, shell_unescape, split_once, trim_blank};

#[test]
fn test_trim_blank() {
Expand Down

0 comments on commit e169da7

Please sign in to comment.