Skip to content
This repository has been archived by the owner on Jun 8, 2024. It is now read-only.

Added support for dot config (.typstfmt.toml) #109

Merged
merged 5 commits into from
Nov 4, 2023
Merged
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ use lexopt::prelude::*;
use typstfmt_lib::{format, Config};

const VERSION: &str = env!("TYPSTFMT_VERSION");
// `DOT_CONFIG_FILE_NAME` is not created as a const due to the fact that we
// would have to duplicate the whole string slice, because
// `format!(".{CONFIG_FILE_NAME}")` (non-const function) cannot be applied to
// `const` (or `static`) values in Rust (1.72.1).
const CONFIG_FILE_NAME: &str = "typstfmt.toml";
const HELP: &str = r#"Format Typst code

Expand Down Expand Up @@ -191,7 +195,18 @@ fn main() -> Result<(), lexopt::Error> {
}

let config = {
if let Ok(mut f) = File::options().read(true).open(CONFIG_FILE_NAME) {
let open_config = |file_name| File::options().read(true).open(file_name);
let config = open_config(CONFIG_FILE_NAME);
let dot_config_file_name = format!(".{CONFIG_FILE_NAME}");
let dot_config = open_config(&dot_config_file_name);
if config.is_ok() && dot_config.is_ok() {
astrale-sharp marked this conversation as resolved.
Show resolved Hide resolved
eprintln!(
"Warning! Both \"{first}\" and \"{second}\" are present. Using \"{first}\".",
first = CONFIG_FILE_NAME,
second = dot_config_file_name
);
}
if let Ok(mut f) = config.or(dot_config) {
let mut buf = String::default();
f.read_to_string(&mut buf).unwrap_or_else(|err| {
panic!("Failed to read config file {CONFIG_FILE_NAME:?}: {err}")
Copy link
Owner

Choose a reason for hiding this comment

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

I feel like now, this can be confusing, can I ask you to fix it by replacing this line
panic!("Failed to read config file {CONFIG_FILE_NAME:?}: {err}")
with
panic!("Failed to read config file {CONFIG_FILE_NAME:?} (or/then .{CONFIG_FILE_NAME:?}): {err}")
Or something else if you have a better idea?

If you're tired of this PR and want to merge already no problem, I can take care of it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about next error:

Config::from_toml(&buf).unwrap_or_else(|e| panic!("Config file invalid: {e}.\nYou'll maybe have to delete it and use -C to create a default config file."))

Error about reading config file should include file name, but TOML parser error shouldn't?

Copy link
Owner

Choose a reason for hiding this comment

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

That's also getting out of date, I didn't notice it. It should include the config file used,

maybe the most simple solution is to, in verbose mode, print which config file is used? But I think it would be nicer if each error contained the name of the file

Copy link
Contributor Author

@Andrew15-5 Andrew15-5 Oct 30, 2023

Choose a reason for hiding this comment

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

We don't even have a verbose mode yet. Ok, I'll update this message too. Everything else can be changed after merging this PR.

Copy link
Owner

Choose a reason for hiding this comment

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

We do have a verbose mode, it's when verbose is true, did you rebase work on master?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohhhh, I checked on the old binary while being on the latest changes. Yes, there is a verbose option. The description says increase verbosity for non errors. Pretty sure that panic!() is an error.

Copy link
Owner

Choose a reason for hiding this comment

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

I meant we could have the error just complain and the verbose option always print the config file used (even on no error)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh (like an init logs), no that's definitely a separate feature.

Copy link
Owner

Choose a reason for hiding this comment

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

👍

Expand Down
Loading