That looks good, I would just move the new methods from bin/rustfmt.rs to config.rs. There is also some logic in main that should be moved to a separate function.

I haven't moved anything yet, what parts do you think should move? TOML merging? Path lookups?

TOML parsing and merging should be in config.rs, as it may be useful if rustfmt is used as a library. The path lookup is more questionnable, do what's the simplest.

Now Config has from_toml which panics and from_toml_merge which returns a result. I think that both should return results, but I didn't change from_toml yet, because it might break stuff.

I think you can remove from_toml completely, since it isn't used any more for rustfmt. We don't yet care about the stability of the library interface. (And at my knowledge, nobody uses rustfmt as a library.)

@nrc Please confirm.

I use it here.

Sorry, I missed that.
I think the best would be a from_toml(toml: &[&str]) -> Result<_>. This would avoid redundancy and make it more generic if we ever want more than two config files.

Instead of having default.toml in the current directory be the default, perhaps it default to a OS-specific config directory, such as /etc/rustfmt.toml? I'm trying to put together a PKGBUILD for rustfmt and I'd like to follow the FHS.

I think we were waiting for a cargo install command to see how we can handle this problem. A hard-coded /etc/rustfmt.toml is fine for installs, but it's annoying for development. We could add some runtime logic to search for ./default.toml, and if not found, /etc/rustfmt.toml, but I feel this should rather be done at build time.
Do you know how this is handled for other softwares ?

@euclio at the moment, the default.toml has to be next to the binary. I chose this because it's the only reliable path. Whatever folder the users choose for the binary will be valid as long as they also have a default.toml next to it. Eventually it should work with cargo install, but for now this seems like a good solution.

It seems cargo install will not track non-binary assets, as detailed in the recently merged RFC: https://github.com/rust-lang/rfcs/blob/master/text/1200-cargo-install.md#non-binary-artifacts

My two cents on the issue: I believe the configuration should be done on a project level and not on a user level. It should therefore be fine to just have a single hardcoded default configuration and look for project files using the strategy that @sbstp has implemented.

We could simply implement Default for Config and use that when no project configuration is found. This would be the most portable solution and we'd no longer need build.rs, which in turn would fix https://github.com/nrc/rustfmt/issues/110.

What happens if the project configuration file is incomplete?

Merging it with the default sounds good to me.

Ok. This is the reason why I kept the default file is because merging at the TOML level when the config is just a hashmap is much easier than merging on the objects. If we can find a clean way of merging the objects, I totally agree with you.

Ah, right. That's an interesting issue. How about we parse the project file into a list of key-value pairs which override the default? I'm doing this in my test configuratikn PR and it seems to work.

-----Original Message-----
From: "Simon Bernier St-Pierre" notifications@github.com
Sent: ‎16-‎8-‎2015 01:13
To: "nrc/rustfmt" rustfmt@noreply.github.com
Cc: "Marcus Klaas de Vries" mail@marcusklaas.nl
Subject: Re: [rustfmt] Add project-specific configuration file support. (#165)

Ok. This is the reason why I kept the default file, it's because merging at the TOML level when the config is just a hashmap is much easier than merging on the objects. If we can find a clean way of merging the objects, I totally agree with you.
—
Reply to this email directly or view it on GitHub.

It would be fine for merging configurations for a later PR and only accept complete configurations for now.

Hey @sbstp, how is this going? Do you need additional input or assistance? It would be great if we could merge this!

Sorry, I forgot. I'm going to rebase it and make the changes, shouldn't be too long.

@marcusklaas I'm not sure what to do with the tests.https://github.com/nrc/rustfmt/blob/master/tests/system.rs#L123 reads from the comments for a config file path. Any clue on the best way to proceed here?

EDIT: I might have something

I've collected issues that this would close or partially close. #110, #72, #136, #119.

This looks great! Whenever the nightly is updated, this should be good to merge. Thank you for your persistence!

One small thing: I think it may be good to have a sample rustfmt.toml file. Maybe in stead of deleting /src/default.toml, we could rename it to /rustfmt.toml?

On the other hand, it may become outdated if we're not careful. What do you think?

Sorry for the message spam, but I think it should be fine. We can point to /tests/config/* for examples.

Yea I was thinking that documenting the config options would be a good idea. Example of the output would be nice.

I'm probably going to work on merging after this, it shouldn't be too hard.

Noyce!

-----Original Message-----
From: "cassiersg" notifications@github.com
Sent: ‎26-‎8-‎2015 22:23
To: "nrc/rustfmt" rustfmt@noreply.github.com
Cc: "Marcus Klaas de Vries" mail@marcusklaas.nl
Subject: Re: [rustfmt] Add project-specific configuration file support. (#165)

Merged #165.
—
Reply to this email directly or view it on GitHub.