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

Enable --rustfmt-bindings by default #1022

Merged
merged 1 commit into from
Sep 25, 2017

Conversation

harlanhaskins
Copy link
Contributor

This patch flips --rustfmt-bindings to --no-rustfmt-bindings and enables
formatting by default. If rustfmt is not accessible, a warning is
printed and the bindings are printed unformatted.

Addresses #977.

@harlanhaskins
Copy link
Contributor Author

harlanhaskins commented Sep 22, 2017

This still has 357 failing tests because the existing expected output is unformatted. For the purpose of this patch, should I update all the test cases or pass --no-rustfmt-bindings instead?

Never mind, looks like I misinterpreted things.

@fitzgen
Copy link
Member

fitzgen commented Sep 22, 2017

This looks good to me -- thanks! Is there still anything outstanding that I'm not seeing that is keeping this PR "WIP"?

@harlanhaskins harlanhaskins changed the title [WIP] Enable --rustfmt-bindings by default Enable --rustfmt-bindings by default Sep 22, 2017
@harlanhaskins
Copy link
Contributor Author

Not really, no! Unless -- should I add the flag back in and mark it deprecated?

@fitzgen
Copy link
Member

fitzgen commented Sep 22, 2017

Not really, no! Unless -- should I add the flag back in and mark it deprecated?

Yeah, let's do that, good idea.

This patch flips --rustfmt-bindings to --no-rustfmt-bindings and enables
formatting by default. If rustfmt is not accessible, a warning is
printed and the bindings are printed unformatted.
@harlanhaskins
Copy link
Contributor Author

@fitzgen Good to merge?

@fitzgen
Copy link
Member

fitzgen commented Sep 25, 2017

@fitzgen Good to merge?

Sorry, didn't see that you pushed new commits -- sometimes I don't get those emails for some reason...

@fitzgen
Copy link
Member

fitzgen commented Sep 25, 2017

@bors-servo r+

Thanks @harlanhaskins !

@bors-servo
Copy link

📌 Commit 89b4dbc has been approved by fitzgen

@bors-servo
Copy link

⌛ Testing commit 89b4dbc with merge 81c634b...

bors-servo pushed a commit that referenced this pull request Sep 25, 2017
Enable --rustfmt-bindings by default

This patch flips --rustfmt-bindings to --no-rustfmt-bindings and enables
formatting by default. If rustfmt is not accessible, a warning is
printed and the bindings are printed unformatted.

Addresses #977.
@bors-servo
Copy link

💔 Test failed - status-travis

@harlanhaskins
Copy link
Contributor Author

Looks like it failed downloading components:
https://travis-ci.org/rust-lang-nursery/rust-bindgen/jobs/279586683

Can we re-run the tests?

@fitzgen
Copy link
Member

fitzgen commented Sep 25, 2017 via email

@fitzgen
Copy link
Member

fitzgen commented Sep 25, 2017 via email

@harlanhaskins
Copy link
Contributor Author

I haven’t deleted the branch...

@fitzgen
Copy link
Member

fitzgen commented Sep 25, 2017

Huh. Retriggered once again. Was seeing

$ cd rust-lang-nursery/rust-bindgen
$ git checkout -qf 81c634b8628b0bf2d34f521a7824d68058a53c8f
fatal: reference is not a tree: 81c634b8628b0bf2d34f521a7824d68058a53c8f
The command "git checkout -qf 81c634b8628b0bf2d34f521a7824d68058a53c8f" failed and exited with 128 during .
Your build has been stopped.

Which is usually what happens when the branch gets deleted before CI finishes.

@fitzgen
Copy link
Member

fitzgen commented Sep 25, 2017

@bors-servo retry

@bors-servo
Copy link

⌛ Testing commit 89b4dbc with merge 9113b7b...

bors-servo pushed a commit that referenced this pull request Sep 25, 2017
Enable --rustfmt-bindings by default

This patch flips --rustfmt-bindings to --no-rustfmt-bindings and enables
formatting by default. If rustfmt is not accessible, a warning is
printed and the bindings are printed unformatted.

Addresses #977.
@bors-servo
Copy link

☀️ Test successful - status-travis
Approved by: fitzgen
Pushing 9113b7b to master...

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