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

Tracking issue: implement versioning #2877

Closed
nrc opened this issue Jul 30, 2018 · 7 comments
Closed

Tracking issue: implement versioning #2877

nrc opened this issue Jul 30, 2018 · 7 comments
Labels

Comments

@nrc
Copy link
Member

nrc commented Jul 30, 2018

As specified in rust-lang/rfcs#2437

cc #2614

@nrc nrc added the blocked Blocked on rustc, an RFC, etc. label Jul 30, 2018
@topecongiro
Copy link
Contributor

So now that rustfmt 1.0 is availble on nightly and will be soon on stable, I think that we need to consider seriously about how we are going to implement versioning. The 'easiest' implementation would be to if guard the new formatting changes after 1.0:

// If we are to change this code,
format_something();

// we have to do something like this:
if rustmft_format_version() >= 1.1 {
    new_way_to_format_something();
} else {
    format_something();
}

However this should be the last resort as it will mess up the code base. Any idea on how to implement this cleanly?

Also what kind of formatting changes need to be blocked between different versions?

@nrc nrc added p-high and removed blocked Blocked on rustc, an RFC, etc. labels Nov 26, 2018
@nrc
Copy link
Member Author

nrc commented Nov 26, 2018

Yes, I have been thinking a bit about this and how to implement it. I was thinking we would add another option for the version and then there is a relatively simple rule that every formatting change has to be gated on an option (either a regular option or the version one). We only need to care about the major version number (i.e., 1 or 2, not 1.0 vs 1.1).

Ideally we would make version-dependent changes at a high level so as not to introduce too many checks. I think we should also be triaging bug fixes with the cost of the fix (in terms of clean code) in mind.

Also once we release 2.0, we can delete all 1.x code, so thinking long term, it is not so bad.

@topecongiro
Copy link
Contributor

Hmm, did things changed from the rfc 2437? Your comment and the initial implementation seems to differ from the original proposal.

@topecongiro
Copy link
Contributor

Ah, I see what you meant. So we only offer two options to users w.r.t. formatting, either using the latest stable at that point (1.0, 2.0, ...) or the absolute latest.

@scampi
Copy link
Contributor

scampi commented Dec 18, 2018

I overlooked this issue.

From what you said @topecongiro in your last comment, then probably a more appropriate change for https://github.com/rust-lang/rustfmt/pull/3250/files#diff-ab15584b47bdf72834e0d64a4cc5f17fR417 would have been something like:

                if context.config.version() == Version::One {
                    ""
                } else if semicolon_for_expr(context, body) {
                    ";"
                };

This is what you did say #3229 (comment) but I had not this conversation in mind...
Should I fix this in a new PR ? Sorry for the back and forth!

@topecongiro
Copy link
Contributor

@scampi I agree that the expression is a bit obscure, but not so much that it requires refactoring. If you are willing to do an extra work here, then the PR is greatly appreciated 😃

@topecongiro
Copy link
Contributor

Closing in favor of #3383.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants