Skip to content
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

Customisation #33

Merged
merged 3 commits into from
Jan 24, 2017
Merged

Customisation #33

merged 3 commits into from
Jan 24, 2017

Conversation

nrc
Copy link
Member

@nrc nrc commented Oct 25, 2016

Customisation of Rustfmt should be allowed (via a rustfmt.toml file) but not encouraged.

Closes #3

enforcement by coercion.

# Alternatives
[alternatives]: #alternatives
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of bikeshedding: Any consideration for .rustfmt.toml? I'm not convinced one or the other is better, but it might be good to list it as an alternative.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both work, as described in the details section

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woops, missed that. I think I need to sleep 💤

# Details
[details]: #details

Users may create a `rustfmt.toml` or `.rustfmt.toml` in their project directory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both? We should pick one. I'd argue for Rustfmt.toml, given Cargo.toml.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@steveklabnik - good for the sake of consistency, but uppercase in file names is sooo annoying. 😢 😉

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The C in Cargo.toml was, apparently, chosen so it'd be sorted near other build system files (Makefile).

rustfmt is not a build system so IMO does not need a capital character.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should all Rust related config file be capitalized then? If not, we should break it now until it's too late.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should break it, or at least not use it for this config. It looks odd. Rubocop's default config is named .rubocop.yml, and CoffeeLint's deafult config is named coffeelint.json. Generally, lowercase is much nicer since it's easier to type IMHO. (The argument for Cargo.toml being sorted similar to Makefile makes sense though.)

@steveklabnik
Copy link
Member

I am not psyched about this but it's what we agreed to, so, I'll just have to deal.

However, code style is an intensely subjective matter and many programmers feel
strongly about it. Furthermore, the Rust community encourages diversity,
individualism, and democracy as part of its culture, and tends not to accept
dictated decisions. It is therefore likely that if Rustfmt only enforced a
Copy link
Member

@aturon aturon Oct 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I have mixed feelings about this paragraph. Rust is not really "democratic" in the typical understanding of the word, but we do closely follow the principles of deliberative democracy via the RFC process. One of the key tenets there is that, after deliberation is complete and a decision is reached, the decision is treated as legitimate and the community abides by it, even if individuals disagree. That's the essence of our RFC process, and the fact that we're using RFCs to determine the standard style means that it's not dictated, but rather deliberated.

I also don't quite follow the logic here: is the concern about the standard style being adopted, or the rustfmt tool being adopted? The "it" in the final sentence is unclear on this point, but it seems important.

You clarify this some in the paragraph below, but don't really spell out why you believe there's more benefit in more widespread rustfmt usage, how you're gauging the relative sizes of these populations, and what role newcomers to Rust play in this breakdown.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the decision is treated as legitimate and the community abides by it, even if individuals disagree.

Strong 👍 to all of this, though especially this bit; it's the sentiment I'm getting at here.

Rustfmt in any configuration, than in a small proportion using it in a single
configuration. We also believe that the best approach to encouraging use of the
default style is to lead by example and exert cultural pressure, rather than
enforcement by coercion.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these beliefs axiomatic, or can they be broken down into more detailed rationale?

@aturon
Copy link
Member

aturon commented Oct 25, 2016

@steveklabnik

I am not psyched about this but it's what we agreed to, so, I'll just have to deal.

I'm still a bit confused about the process here -- AFAIK, nothing has been decided/approved at this point, since this question is still at the RFC stage, right? (In particular the "FCP" for issues prior to making an RFC has left me a bit confused about the process mechanics.)

@nrc
Copy link
Member Author

nrc commented Oct 25, 2016

@aturon

I'm still a bit confused about the process here -- AFAIK, nothing has been decided/approved at this point, since this question is still at the RFC stage, right? (In particular the "FCP" for issues prior to making an RFC has left me a bit confused about the process mechanics.)

Maybe we should drop the FCP for issues, I feel it might be superfluous as well as confusing.

Let me outline the process again:

  • make an issue
  • get rough consensus (as indicated by conclusion of FCP)
  • do any necessary implementation
  • make a PR
  • get final consensus (again, indicated by FCP).

This issue/PR doesn't need any implementation, so the process is a bit overkill, but the intention is to avoid doing speculative Rustfmt implementation work. I feel that we need some implementation before the RFC PR (c.f., regular RFCs) because I have found that in the past, broad formatting rules fall down in the details which are not apparent until run on large code samples.

Where Steve says "what we agreed to", I believe he is referring to a discussion we had at the style meeting where we agreed on the way forward I've written up here (prior to issue FCP, though that was fairly uneventful).

To be clear, we have not decided on the issues here until the RFC PR is accepted. Agreement/conclusion of FCP on the issue is agreement to move to this stage.

@steveklabnik
Copy link
Member

Where Steve says "what we agreed to", I believe he is referring to a discussion we had at the style meeting where we agreed on the way forward I've written up here (prior to issue FCP, though that was fairly uneventful).

Yes, this is what I mean. That is, as a member of the style team, we have reached consensus among ourselves, even though I was ultimately a dissenter here. So, making a strong argument against the team feels bad, but this issue is possibly the one I care most about, so I still feel the need to register a small "I'm not blocking this but I'm not super happy about it" so that I don't regret it forever 😉

It is true that this is ultimately not decided, and if strong arguments are presented, this could turn around. The style team's decision isn't final or anything. That's the whole point of having this discussion!

@WiSaGaN
Copy link

WiSaGaN commented Nov 3, 2016

I am wondering if we can actually get some numbers on sentiment of using style one does not agree with. Do a lot of people hate it, or most people can live with it? In general I feel we shouldn't force people to adopt things by not allowing people to choose otherwise, because there may be legitimate case only the alternative works however few there may be. On the other hand, although people do have different tastes about the style, I can't think of a case that using a specific style can be proven to be big inconvenience or deal-breaker for the actual work. It should mostly come down to people's opinion of adopting to a style they are not happy with, at least at the start. Do a lot of people really hate to use a style that they don't like so they go on with a fork or write a new tool if they are not provided the satisfactory option, or most people can live with it and the initial dislike eventually goes away? And what is the chance of our final agreed-upon style is disliked by a good portion of people? The last question may be a bit hard to get answer to. The first one may be done in some form of survey.

@bruno-medeiros
Copy link

@WiSaGaN I guess it depends on the particular aspects/options of the style. I have preferences with regards to indent size, indent style (tabs vs spaces), max line width, and several other customization points - but in nearly all of them I would accept a style choice that is not my preference if that's the recommended style. There are no preferences I feel strongly about - the only exception so far is alignment indentation, aka visual indentation (#8) which I think should be killed with fire. It's bad enough to discourage me (to a degree) on working on other people's code that use that indentation. And if rustfmt only indented this way, I'd certainly want a fork that could indent with block indentation too, so I could use it in my code.

@fabric-and-ink
Copy link

I agree with @bruno-medeiros. There needs to be a way to tweak certain things. At least for the time I work alone/in a small private team on a project. When it goes open source I can agree to switch the style to the chosen one. But for my private use it's a different thing.

@elahn
Copy link

elahn commented Nov 9, 2016

I'm alright with adopting a style for the sake of getting along. What I'm against is prohibiting the use of a better style because it doesn't yet have ubiquitous tooling support.

With customisation and git hooks, people can work in a codebase formatted to their preferences, yet commit in the agreed upon style.

@chriskrycho
Copy link

Throwing this out there for discussion: editors generally let you set your own preferences for many of the preferential things discussed in this and other threads. Why does rustfmt need to be the tool that supports those customizations?

I recognize one scenario: you may want to reformat while working, and then reformat again at commit; that seems like an extraordinary amount of pain, but to each their own.

Are there others?

Also, when people say "there needs to be a way" I think it's worth remembering that what you're saying is "I would super-strongly prefer it"—maybe even to the "I would choose not to use such a tool"; but "needs" is an extremely strong claim. I think claims about what is necessary for the tool ought to be backed up by discussions of how it impacts the community, and not merely about what our individual preferences are (however strong). That is:

What are the specific costs and benefits to the community of having a single style, vs. the costs and benefits to the community of having a bunch of ways to tweak things to individual preference?

@est31
Copy link
Member

est31 commented Nov 10, 2016

@chriskrycho You assume that by removing customizability of the format tool you magically make the community adopt one style. This is wrong.

If rustfmt won't support my preferred style, I'll maybe fork it, maybe I will just simply do manual formatting, putting additional load on my contributors (instead of telling them to "run this tool" they now have to manually fix the style). But I won't use the community style just because there is no format tool for my personal style, or because the most popular format tool can't be customized.

@chriskrycho
Copy link

@est31:

You assume that by removing customizability of the format tool you magically make the community adopt one style. This is wrong.

Nope, I assume that if the community were to agree on a single style, rustfmt would be the way we'd apply it, which is quite different. And that's the point of the question I asked.

@nrc
Copy link
Member Author

nrc commented Nov 10, 2016

editors generally let you set your own preferences for many of the preferential things discussed in this and other threads. Why does rustfmt need to be the tool that supports those customizations?

It is common for editors to use Rustfmt to format Rust code, so if we want these options in the editor, then they must also be in Rustfmt.

@brson
Copy link

brson commented Nov 14, 2016

This RFC reads well and I'm fine with this approach. Nice RFC @nrc!

Since other styles are already in use, rustfmt is already capable of it, and because rustfmt can use options anyway for doing our own style comparisons, it seems reasonable to have best-effort support for some other styles. It may be preferable to have a universal style, but that would have been easier to do under different circumstances, and not accomplishing that doesn't both me too much.

I do worry that more options means more maintenance and more breakage.

@solson
Copy link
Member

solson commented Nov 14, 2016

I do worry that more options means more maintenance and more breakage.

This is my primary concern. I'd like to support customization as well, but the specific options should be decided on a case-by-case basis.

On every RFC discussion where a non-standard option comes up, the community needs to weigh whether or not it's worth complicating rustfmt. Sometimes it'll be worth it, sometimes not.

@briansmith
Copy link

One of the key tenets there is that, after deliberation is complete and a decision is reached, the decision is treated as legitimate and the community abides by it, even if individuals disagree. That's the essence of our RFC process, and the fact that we're using RFCs to determine the standard style means that it's not dictated, but rather deliberated.

I think that's true is the question is "Are people going to accept what is decided as the default or official style?" But, it's not true if the question is "Are people going to accept what is decided as-is for use in their own projects?" In the end, I expect a lot of people will customize the default style, whether through rustfmt.toml or by asking contributors to use a forked version of rustfmt.

@nrc
Copy link
Member Author

nrc commented Dec 2, 2016

I'd like to move to FCP here - I propose adding some of the discussion in the comments to the RFC as additional motivation/background, but I don't think there any substantial changes to make.

@solson
Copy link
Member

solson commented Dec 2, 2016

The one unresolved conversation I'm aware of is what to call the config file: #33 (comment)

@nrc
Copy link
Member Author

nrc commented Dec 2, 2016

I'm thinking that how a formatter is customised should be up to the formatter, not the style guide. So, e.g., nrcfmt could be standards compliant and use nrc.xml as input, as long as the options that can be customised match the ones we specify for rustfmt.

@perlun
Copy link

perlun commented Dec 6, 2016

So, e.g., nrcfmt could be standards compliant and use nrc.xml as input, as long as the options that can be customised match the ones we specify for rustfmt.

But if its defaults are different to the defaults provided in the style guide, it would be considered non-compliant, right? So "the defaults" would be both the list of customizable options, as well as the default values for these options.

@nrc
Copy link
Member Author

nrc commented Dec 18, 2016

But if its defaults are different to the defaults provided in the style guide, it would be considered non-compliant, right? So "the defaults" would be both the list of customizable options, as well as the default values for these options.

Yes. But the way in which the formatter chooses to specify the options and defaults is unspecified.

@est31 est31 mentioned this pull request Dec 19, 2016
Copy link

@perlun perlun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The typo must be fixed before merge; the other comments are more subjective in nature.

# Summary
[summary]: #summary

Customisation of Rustfmt should be allowed (via a `rustfmt.toml` file), but not
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit of a nit: should we really be referring to rustfmt as Rustfmt? (I see that https://github.com/rust-lang-nursery/rustfmt also does the same)

I find it a bit confusing, since the actual binary you invoke is not Rustfmt but rustfmt. It would feel more consistent to refer to it by the latter form everywhere. Has this been discussed previously and settled to use the initial-capital form?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think of 'Rustfmt' as the name of the abstract tool, and thus (like all names) should be capitalised. rustfmt is the name of the executable and coincidentally has the same name. Similarly we talk about 'Rust' the language, but rustc the command line.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that my opinion matters much here but I agree regarding rustfmt. The tool (and repo) is named rustfmt so that solidifies the name to me.

[details]: #details

A formatter such as Rustfmt may be customised by the user. These customisations
may be saved for a project using a cusomtisation file. For example, Rustfmt can
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling: cusomtisation vs customisation.

A formatting tool may be customised in other ways, but must stick to the options
and defaults specified by the style RFCs.

Customisation will be documented, but explicitly discouraged.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we go from "not encouraged" to "explicitly discouraged", a somewhat stronger form of "non-encouragement".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think explicit discouragement is what we want. I'll try and make sure this is consistent throughout.

[implementation]: #implementation

Rustfmt already allows customisation via `rustfmt.toml`. There is nothing more
to implement here. Over time, I expect the number of options to be reduced.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it's a noble desire, where I can understand the background thinking for it, I do think the latter sentence represents somewhat unrealistic expectations; I would vote for removing that part. (The number of options are more likely to increase as the adoption rate for Rust increases, or at least stay more-or-less constant. Expecting it to be reduced is expecting people to think more and more the same as time goes by; I'm not sure that's a realistic way of thinking.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that the RFC process should lead to a reduction, I agree that as time goes by there is likely to be a natural increase. I'll clarify.

@nrc nrc merged commit 58f1c53 into master Jan 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.