-
-
Notifications
You must be signed in to change notification settings - Fork 140
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
add a config file at ~/.config/comrak/config #157
Conversation
--smart Use smart punctuation | ||
--unsafe Allow raw HTML and dangerous URLs | ||
-V, --version Prints version information | ||
|
||
OPTIONS: | ||
-c, --config-file <PATH> Path to config file containing command-line arguments [default: | ||
/Users/kivikakk/.config/comrak/config] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the documentation should show ~/.config/comrak/config
? Or is the --help output dynamic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output is dynamic, yeah — it'll show the actual default based on their XDG config path. The README excerpt is generated from running the actual binary on my machine, so it shows mine as a sample. :)
README.md
Outdated
@@ -45,11 +45,14 @@ FLAGS: | |||
--github-pre-lang Use GitHub-style <pre lang> for code blocks | |||
--hardbreaks Treat newlines as hard line breaks | |||
-h, --help Prints help information | |||
--no-config-file Do not read config file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens in the case of --no-config-file --config-file ./config
? I assume --no-config-file
takes precedence? Would it be just as well to have --config-file false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, --no-config-file
takes precedence. Hesitant to use false
since it's not a boolean per se, but I do like not having the extra option, so will make none
recognised!
@@ -173,3 +220,15 @@ fn main() -> Result<(), Box<dyn Error>> { | |||
|
|||
process::exit(0); | |||
} | |||
|
|||
fn get_default_config_path() -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at https://github.com/whitequark/rust-xdg/blob/master/src/lib.rs I thought that this might not handle windows, but following through to https://crates.io/crates/dirs, it looks like it is cross-platform after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Rust libraries have been pretty good at Windows compat in my experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, actually. Nope. >_< Missed the first line:
#![cfg(any(unix, target_os = "redox"))]
This doesn't work on Windows at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do something like this: https://github.com/DanielKeep/cargo-script/blob/614e60e5932e218ebff1e471303eb0d59870d03b/src/platform.rs#L309-L363
But for now I'm defaulting to none
on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Awesome, thanks for your careful review! I'll do a release shortly. |
Fixes #155.
/cc @solderjs — what do you think?