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

Try shorthand #893

Merged
merged 3 commits into from
May 12, 2016
Merged

Try shorthand #893

merged 3 commits into from
May 12, 2016

Conversation

marcusklaas
Copy link
Contributor

This is based off #892.

Addresses #867. I'm not 100% content with the implementation, but would like to get some feedback on the output before refining it.

r? @kamalmarhubi, @nrc?

@@ -148,6 +148,7 @@ impl Rewrite for ast::Expr {
ast::ExprKind::Closure(capture, ref fn_decl, ref body) => {
rewrite_closure(capture, fn_decl, body, self.span, context, width, offset)
}
// ast::ExprKind::Try(..) |
Copy link
Contributor

Choose a reason for hiding this comment

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

is this meant to be commented out?

let yyyy = expr?.another?.another?.another?.another?.another?.another?.another?.another?.test();
let zzzz = expr?.another?.another?.another?.another?;
let aaa = x??????????????????????????????????????????????????????????????????????????;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

output looks ok to me!

@nrc
Copy link
Member

nrc commented Mar 31, 2016

looks good to me. I think the output looks good. Re the FIXME, I think we should never separate the ? from the subject, i.e., we can try to break the subject, or we can fail, but we shouldn't insert a newline before the ?.

@erikjohnston
Copy link
Contributor

I was trying this and got a panic when using it as the last expression in a block, e.g.:

fn test() {
    a?
}
Request to format inverted span: Loc { file: FileMap(tests/source/chains.rs), line: 127, col: CharPos(1) } to Loc { file: FileMap(tests/source/chains.rs), line: 127, col: CharPos(0) }

(where line 127 is the last line in the example)

@marcusklaas
Copy link
Contributor Author

Thanks for the test case @erikjohnston, I'll see if I can fix it!

@marcusklaas
Copy link
Contributor Author

Turned out to be a rustc bug. This PR will have to wait until rust-lang/rust#32711 lands and syntax_syntex is updated.

@marcusklaas
Copy link
Contributor Author

The Rust PR (finally) landed. Shouldn't be too long until a syntex update.

@marcusklaas
Copy link
Contributor Author

Updated. we now also convert uses of try! to the shorthand ? when the relevant option is set. This may be the first case of AST manipulation in rustfmt!

Tests won't pass yet -- still need syntex update.

@nrc
Copy link
Member

nrc commented May 1, 2016

@marcusklaas hey, are the changes in syntex now? Would be great to land this, I'd like to release 0.5 soon, and it would be great for this to be part of it (need it for formatting stuff in the Rust repo, amongst other things).

@kamalmarhubi
Copy link
Contributor

I just made #971 to bump to syntex 0.31. Not sure if it includes the change but just in case. :-)

@marcusklaas
Copy link
Contributor Author

I don't think it does - but the next version should.. Just a bit longer!

@kamalmarhubi
Copy link
Contributor

@marcusklaas have a syntex_syntax bump: #979 :-)

@marcusklaas
Copy link
Contributor Author

marcusklaas commented May 9, 2016

Thanks for the syntex update @kamalmarhubi. I'll see if I can rebase this :-)

@marcusklaas
Copy link
Contributor Author

marcusklaas commented May 12, 2016

Yo @nrc, this should be ready to merge now. I've also implemented a break in assignments when it's more compact. It should mitigate the need for tab indentation in chains; I'll look into that a bit more.

@marcusklaas
Copy link
Contributor Author

Closes #985.

@marcusklaas
Copy link
Contributor Author

Supersedes #751 too.

@marcusklaas
Copy link
Contributor Author

Addresses #574 as well.

ast::ExprKind::Try(_) => (),
_ => result.push_str(connector),
};
result.push_str(&rewrite[..]);
Copy link
Member

Choose a reason for hiding this comment

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

nit: probably don't need the [..]

@nrc nrc merged commit c1949f5 into rust-lang:master May 12, 2016
@marcusklaas marcusklaas deleted the try-shorthand branch May 13, 2016 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants