New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config: Introduce a mergeable config #801

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@kamalmarhubi
Copy link
Contributor

kamalmarhubi commented Feb 2, 2016

This pull request introduces a mergeable config abstraction, and uses it for the verbose and skip_children configuration options.

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor

kamalmarhubi commented Feb 2, 2016

@nrc, this is much nicer than #795 imo :-)

@kamalmarhubi kamalmarhubi force-pushed the kamalmarhubi:mergeable-config branch 2 times, most recently from 8894c3d to c673723 Feb 2, 2016

kamalmarhubi added some commits Feb 2, 2016

config: Introduce a mergeable config
This commit replaces `ParsedConfig` with a `PartialConfig` that can be
merged into a `Config` or another `PartialConfig`. This provides a
unified place for configuration that is passed through rustfmt, and a
mechanism for overriding settings from other sources.

Expected uses:
 - overriding config file options from command line options
 - adding options that do not make sense in the config file, such as
   line ranges to restrict formatting to; see #434

refs #434

@kamalmarhubi kamalmarhubi force-pushed the kamalmarhubi:mergeable-config branch from c673723 to 9e98191 Feb 2, 2016

config: Use PartialConfig for write_mode
This changes the handling of `write_mode` to use `PartialConfig` and
`Config::merge()` for overriding, and to no longer thread it through the
formatting code as a separate parameter.
@nrc

This comment has been minimized.

Copy link
Member

nrc commented Feb 3, 2016

What is the advantage of using a PartialConfig over just initialising a regular Config with the defaults and then overriding them?

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor

kamalmarhubi commented Feb 3, 2016

Part of the motivation was to tidy up how write_mode is passed around and dealt wtih. I realise I didn't put up that change; pushed it up in case you want to see it.

On reflection I don't feel too strongly about this change, so it's up to you. The write_mode setting is kind of overloaded in a way that this doesn't fix anyway.

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor

kamalmarhubi commented Feb 3, 2016

Anyway, feel free to look over the changes moving write_mode to only come from Config. I'll hold off fixing conflicts till I hear something positive / negative about the change. Thanks!

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor

kamalmarhubi commented Feb 3, 2016

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Feb 4, 2016

So, having the write mode be part of the config sounds like a good change. However, I don't see that using the builder pattern for the config is a pre-req for it. Maybe I'm missing something, but couldn't you just set it to some default and then reset it later in the right places?

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor

kamalmarhubi commented Feb 5, 2016

Yeah, the config stuff isn't necessary. I'll send another PR tomorrow that gets the write_mode read solely from the config.

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor

kamalmarhubi commented Feb 5, 2016

#812 replaces this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment