Thanks for the PR! Do I understand correctly that there are now two enums for the write mode? We'd have just one, ideally. Also, it's no longer possible to set the write mode to Replace with command line parameters once the write mode is specified in the config, right? I think the best way to solve this is to have a distinct Default variant, which maps to <WriteMode as Default>::default().

Firstly, no problem since this is something I want for my projects as well.

To answer some of your questions:

• Yes there are 2 enums, which is why I referred to it as a hack. I would also like just one but the parsing code was a bit much for me as it would need a change so that it could support the NewFile(&'static str) option in lib.rs' WriteMode.
• Yes, I did so because when the write-mode param is not passed in, Replace is what is assumed.
• Where would we put Default? In lib.rs' WriteMode?

I'd say we drop the NewFile variant. I can't think of any use for it and doubt any one is using it. It doesn't work well when multiple files are involved either. @nrc, what do you think?

In that case we should be able to re-use WriteMode in lib.rs. We'd just need an invocation of impl_enum_decodable to be able to parse it.

If we are going to drop variants, we should take the opportunity and check that they are all needed so that we only need to make this change once.
Also I don't know if I understood this correctly but would the config WriteMode be taken as the Default variant for WriteMode?

The other ones all have their use, I think. WriteMode::Return is mostly useful when rustfmt is used as a library and not so much as a binary. To remove it would require some non-trivial refactoring, so I'd leave it for now.

I was looking at how to go about removing the second enum and found that WriteMode implements FromStr, which is also implemented in impl_enum_decodable!, however the current implementation returns an error when Return is converted. Is this intended?

Yea, because it shouldn't ever be used by users of the binary.

How can this function be maintained across the impl_enum_decodable! call?

It cannot. I guess we'll have to remove the Return as a variant after all. Sorry for the confusion.

Ok, no problem. Right now what I did with default is to make a variant so that if both the config and the argument are default it will use Replace but if there is a argument it will prefer that, however in filemap.rs there is a match against WriteMode, how should I implement the case where Default is the mode? Which should never happen in the execution of the program, a panic!?

You can use unreachable!() there. If, by some bug, it would be evaluated in runtime the program will panic. It signals to the reader that we should never go there.

Got it, that is what I ended up writing since it also ends up as a panic. Should I put those changes as a new commit or rebase the current one?

A rebase would be cleanest, I think.

Should I keep WriteMode in lib.rs or move it to config since at this point using configuration_option_enum results in the same code?

Both make sense to me. Either way is fine!

Opps... Forgot to change tests... Many of them use Return

Ok so many of the tests use the Return option, not quite sure how to handle this, any ideas?

Yea, that's the rough bit. The basic idea I had was to have a public function that returns a HashMap from file names (Strings) to formatted code (Strings). The tests would use that.

The binary could use that function to either write the results to stdout or file, depending on WriteMode.

This is actually a change I've been wanting to do for a while, so I wouldn't mind finishing up your PR (by making a PR for your branch). If you're willing to give it a try, that'd be even cooler. Let me know.

I would be fine with the branch idea, at the end all I want is for the feature to be there.

However I did think of doing a check on the run function so that it panics if anyone uses return there, not sure if that is a good way around it though, however the tests never run correctly on my machine(Windows 8), even when they pass in Travis.

That function also sounds like a nice abstraction on the write_file fn so if you want to use the opportunity to do that go ahead and create the branch and I'll reopen to that branch

Ah, that may be #617. I'm not sure how to fix that as I'm not on a Windows machine myself..

I'll try to make a PR to your branch tonight. Thanks for your changes!

No problem! Thank you very much for your comments

I'll just create a PR to your master branch ;-)

Just a note, I just realized that Default is a trait... I did not know that, however the code works better having an implicit Default variant since it gets rid of the ambiguity of Replace

Agreed!

Created a pull request for your branch here: https://github.com/svmnotn/rustfmt/pull/1.

Hmmm, one unintended consequence seems to be that write modes are now case sensitive. That's probably not what we want.

yeah... I ran into that while testing but I think so are line endings and everything using the derived fromStr (I think, I need to test that)

Just tested, line endings are also case sensitive

The line endings option, you mean?

yes, sorry I was looking at the code, I'll branch off and see if maybe I can PR that in. Hopefully it will be less problematic ;)

#739 fixes the issue pointed out by @marcusklaas

@marcusklaas is this landable now?

@svmnotn needs a rebase