Add project-specific configuration file support. #165

sbstp · 2015-07-31T23:27:46Z

It's now possible to create a file relative to a project that
will override the default values. That file is called "rustfmt.toml".
The default values are stored in a file called "default.toml" and
are expected to be in the same folder as the binary, regardless
of where the binary is stored.

A "default.toml" is still written next to the manifest because
the test suite needs it.

cassiersg · 2015-08-01T09:34:50Z

src/bin/rustfmt.rs

+
+use toml::Parser;
+
+// Tries to read a file and parse the conents as a toml table. Panics on error.


nit: contents

cassiersg · 2015-08-01T09:59:18Z

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.

cassiersg · 2015-08-01T10:00:19Z

src/bin/rustfmt.rs


-    run(args, WriteMode::Overwrite, &def_config);
+    let def_path = get_default_file();
+    println!("Default config file: {}", def_path.display());


Should be info!, I think.

The project doesn't have the log crate, idk if I should add it.

We are already using rust-lang/log.

sbstp · 2015-08-01T20:15:43Z

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

cassiersg · 2015-08-01T20:32:23Z

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.

cassiersg · 2015-08-01T20:40:47Z

src/bin/rustfmt.rs

+        None => {
+            let mut errmsg = String::new();
+
+            try!(writeln!(errmsg, "TOML error in file {}", path.as_ref().display()));


The try! here is misleading, since the writeln! will never fail: let mut errmsg = format!("TOML error in file {}\n", path.as_ref().display());

If it won't fail I can just put a let _ = ... in front of it to stop the warning.

sbstp · 2015-08-02T01:10:46Z

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.

cassiersg · 2015-08-02T09:05:58Z

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.

sbstp · 2015-08-02T17:08:31Z

I use it here.

cassiersg · 2015-08-04T17:45:02Z

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.

euclio · 2015-08-09T00:08:25Z

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.

cassiersg · 2015-08-09T08:47:14Z

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 ?

sbstp · 2015-08-09T19:23:26Z

@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.

marcusklaas · 2015-08-15T22:30:16Z

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

marcusklaas · 2015-08-15T22:39:51Z

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.

sbstp · 2015-08-15T23:08:05Z

What happens if the project configuration file is incomplete?

marcusklaas · 2015-08-15T23:10:17Z

Merging it with the default sounds good to me.

sbstp · 2015-08-15T23:13:15Z

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.

marcusklaas · 2015-08-15T23:25:23Z

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" [email protected]
Sent: ‎16-‎8-‎2015 01:13
To: "nrc/rustfmt" [email protected]
Cc: "Marcus Klaas de Vries" [email protected]
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.

marcusklaas · 2015-08-16T12:37:03Z

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

marcusklaas · 2015-08-25T20:14:02Z

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

sbstp · 2015-08-25T21:00:17Z

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

rust-lang/rust#27857

sbstp · 2015-08-25T22:48:30Z

@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

sbstp · 2015-08-25T23:18:39Z

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

marcusklaas · 2015-08-26T18:05:25Z

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?

marcusklaas · 2015-08-26T18:08:14Z

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

marcusklaas · 2015-08-26T18:10:06Z

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

sbstp · 2015-08-26T18:37:34Z

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

sbstp · 2015-08-26T18:51:37Z

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

Add project-specific configuration file support.

marcusklaas · 2015-08-26T20:24:46Z

Noyce!

-----Original Message-----
From: "cassiersg" [email protected]
Sent: ‎26-‎8-‎2015 22:23
To: "nrc/rustfmt" [email protected]
Cc: "Marcus Klaas de Vries" [email protected]
Subject: Re: [rustfmt] Add project-specific configuration file support. (#165)

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

cassiersg reviewed Aug 1, 2015
View reviewed changes

Fix build on nightly

6adb6a1

rust-lang/rust#27857

Add project-specific configuration file support

9ead471

Delete src/default.toml file

99b0aa9

cassiersg added a commit that referenced this pull request Aug 26, 2015

Merge pull request #165 from SBSTP/config

0957f31

Add project-specific configuration file support.

cassiersg merged commit 0957f31 into rust-lang:master Aug 26, 2015

sbstp deleted the config branch August 26, 2015 21:16

cassiersg mentioned this pull request Aug 26, 2015

Rustfmt always recompiles #110

Closed


		use toml::Parser;

		// Tries to read a file and parse the conents as a toml table. Panics on error.

Add project-specific configuration file support. #165

Add project-specific configuration file support. #165

Uh oh!

Conversation

sbstp commented Jul 31, 2015

Uh oh!

cassiersg Aug 1, 2015

Choose a reason for hiding this comment

Uh oh!

cassiersg commented Aug 1, 2015

Uh oh!

cassiersg Aug 1, 2015

Choose a reason for hiding this comment

Uh oh!

sbstp Aug 1, 2015

Choose a reason for hiding this comment

Uh oh!

cassiersg Aug 1, 2015

Choose a reason for hiding this comment

Uh oh!

sbstp commented Aug 1, 2015

Uh oh!

cassiersg commented Aug 1, 2015

Uh oh!

cassiersg Aug 1, 2015

Choose a reason for hiding this comment

Uh oh!

sbstp Aug 1, 2015

Choose a reason for hiding this comment

Uh oh!

sbstp commented Aug 2, 2015

Uh oh!

cassiersg commented Aug 2, 2015

Uh oh!

sbstp commented Aug 2, 2015

Uh oh!

cassiersg commented Aug 4, 2015

Uh oh!

euclio commented Aug 9, 2015

Uh oh!

cassiersg commented Aug 9, 2015

Uh oh!

sbstp commented Aug 9, 2015

Uh oh!

marcusklaas commented Aug 15, 2015

Uh oh!

marcusklaas commented Aug 15, 2015

Uh oh!

sbstp commented Aug 15, 2015

Uh oh!

marcusklaas commented Aug 15, 2015

Uh oh!

sbstp commented Aug 15, 2015

Uh oh!

marcusklaas commented Aug 15, 2015

Uh oh!

marcusklaas commented Aug 16, 2015

Uh oh!

marcusklaas commented Aug 25, 2015

Uh oh!

sbstp commented Aug 25, 2015

Uh oh!

sbstp commented Aug 25, 2015

Uh oh!

sbstp commented Aug 25, 2015

Uh oh!

marcusklaas commented Aug 26, 2015

Uh oh!

marcusklaas commented Aug 26, 2015

Uh oh!

marcusklaas commented Aug 26, 2015

Uh oh!

sbstp commented Aug 26, 2015

Uh oh!

sbstp commented Aug 26, 2015

Uh oh!

marcusklaas commented Aug 26, 2015

Uh oh!

Uh oh!