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

Conversation

Andrew15-5
Copy link
Contributor

I don't know whether we should create dot file by default (-C) or not (I think a lot of formatters do create dot file by default), but support for reading from dot file should exist. The order/priority of reading config files (from highest to lowest): typstfmt.toml.typstfmt.toml.

@astrale-sharp
Copy link
Owner

astrale-sharp commented Oct 3, 2023

small nitpick, could you please:

  • remove DOT_CONFIG_FILE_NAME and just inline format(".{CONFIG_FILE_NAME}") in the code with a comment explaining that it happens above CONFIG_FILE_NAME?
  • If you have the time, it would be great if when both the CONFIG_FILE_NAME and DOT_CONFIG_FILE_NAME are present, a warning is printed?

Thank you for this!

@Andrew15-5
Copy link
Contributor Author

  • remove DOT_CONFIG_FILE_NAME and just inline format(".{CONFIG_FILE_NAME}")

This is why I've done this the way I've done this:

cannot call non-const fn `typstfmt_lib::format` in constants
calls in constants are limited to constant functions, tuple structs and tuple variants (rustc E0015)
cannot call non-const fn `std::fmt::format` in constants
calls in constants are limited to constant functions, tuple structs and tuple variants (rustc E0015)
  • when both the CONFIG_FILE_NAME and DOT_CONFIG_FILE_NAME are present, a warning is printed

I know about makefile (from meme & comments), which has priority over Makefile, so I assumed that we can do the same (dot file is like `Makefile). But I also can see the problem from a user POV (which for some unknown reason have 2 config files in the same dir). I can do this, but are there any specific "requests" right away?

@astrale-sharp
Copy link
Owner

This is why I've done this the way I've done this:

cannot call non-const fn `typstfmt_lib::format` in constants
calls in constants are limited to constant functions, tuple structs and tuple variants (rustc E0015)

My bad, it's format!

@astrale-sharp
Copy link
Owner

I know about makefile (from meme & comments), which has priority over Makefile, so I assumed that we can do the same (dot file is like `Makefile). But I also can see the problem from a user POV (which for some unknown reason have 2 config files in the same dir). I can do this, but are there any specific "requests" right away?

What do you mean requests?

I just think it's an error if anyone had both .typstfmt.toml and typstfmt.toml in their directory structure.
They might try to modify the dot version and wonder why no change is visible

@Andrew15-5
Copy link
Contributor Author

My bad, it's format!

I already tried it:

cannot call non-const fn `std::fmt::format` in constants
calls in constants are limited to constant functions, tuple structs and tuple variants (rustc E0015)

const DOT_CONFIG_FILE_NAME: String = format!(".{CONFIG_FILE_NAME}");

I already faced this problem. We either have to import a crate like lazy_static or duplicate the whole string slice.

This gives the same error: const CONFIG_FILE_NAME: &str = &DOT_CONFIG_FILE_NAME[1..];.

const CONFIG_FILE_NAME: &str = &DOT_CONFIG_FILE_NAME.get(1..).unwrap(); gives Option::<T>::unwrap is not yet stable as a const fn (rustc).

I just think it's an error if anyone had both .typstfmt.toml and typstfmt.toml in their directory structure.
They might try to modify the dot version and wonder why no change is visible

Ok, I'll show a warning in stderr.

@astrale-sharp
Copy link
Owner

I didn't mean DOT_CONFIG_FILE_NAME = "." + format!(...)
I meant, remove DOT_CONFIG_FILE_NAME and inline the above logic

src/main.rs Outdated Show resolved Hide resolved
@astrale-sharp
Copy link
Owner

Feel free to tell me when you're ready for another review!

@Andrew15-5
Copy link
Contributor Author

All done.

src/main.rs Outdated
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.

👍

@astrale-sharp
Copy link
Owner

Thanks for this contribution :)
It's good to go, I'm overreaching :p

src/main.rs Outdated Show resolved Hide resolved
@Andrew15-5
Copy link
Contributor Author

It's good to go

Are you sure the global config feature on master would work with this? I need to check.

@astrale-sharp
Copy link
Owner

astrale-sharp commented Oct 30, 2023

I read the code again and it would have a weird behavior indeed

I think it needs to be rewritten as (pseudocode)

if let Ok(mut f) = File::options().read(true).open(CONFIG_FILE_NAME)
else  if let Ok(mut f) = File::options().read(true).open(DOT_CONFIG_FILE_NAME)
else /* try global file */
else default

Changed only error messages that are related to the reading/parsing of a config file.
@Andrew15-5
Copy link
Contributor Author

I read the code again and it would have a weird behavior indeed

I think it needs to be rewritten as (pseudocode)

if let Ok(mut f) = File::options().read(true).open(CONFIG_FILE_NAME)
else  if let Ok(mut f) = File::options().read(true).open(DOT_CONFIG_FILE_NAME)
else /* try global file */
else default

Nah, I also read the code and it looks fine. If global isn't loaded, then the program will panic. else default is redundant. Also, making a whole separate outer else if is too much. We have the same logic and same errors for both, so I only included a small if to get the correct file name to throw an error, if necessary.

@astrale-sharp
Copy link
Owner

Looks really nice! Good job!
I'm ready to merge whenever you are !

@astrale-sharp astrale-sharp merged commit 6ab2cab into astrale-sharp:master Nov 4, 2023
1 check passed
@astrale-sharp
Copy link
Owner

Thanks a ton !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants