Skip to content

Add GitHub Action to test master rustfmt formatting vs a feature branch #5479

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

Merged
merged 1 commit into from
Aug 13, 2022

Conversation

ytmimi
Copy link
Contributor

@ytmimi ytmimi commented Aug 1, 2022

This new action is intended to help us maintainers determine when feature branches cause breaking formatting changes by running rustfmt (master) and the feature branch on various rust repositories.

Over time I expect the list of checked projects to increase.

With this action in place we can more easily test that a new feature or bug fix doesn't introduce breaking changes. Although this action needs to be manually triggered right now, we might consider adding it to our CI runs in the future.

To run the new action we can head over to the actions tab and click Run workflow to supply the inputs that the script expects.
Screen Shot 2022-08-01 at 1 28 10 AM

For example to test out the proposed fn_call_layout changes (#5337) you could supply the following form arguments.

https://github.com/ytmimi/rustfmt.git
issue_5218

I'm far from a bash pro, so if you have any suggestions for how to simplify the script those would be greatly appreciated 🙏🏼

@calebcartwright
Copy link
Member

I like the sounds of this! Haven't had a chance to look yet, but curious if/how readily it could be used to test a branch against currently released versions of rustfmt (e.g. branch vs stable or nightly)

@ytmimi
Copy link
Contributor Author

ytmimi commented Aug 2, 2022

At the moment the script doesn't support that, but I think it should be pretty easy to add an optional input to specify a git tag to checkout after we do a git clone on the rustfmt repo!

Just as a heads up, GitHub actions that are triggered on workflow_dispatch can only be run if they're on the default branch for that repo. To test this action on my feature branch I temporarily set my diff_check_script as my default branch for my fork of rustfmt.

Here are some links to workflow runs that I already tested so you can get a feel for the output:

  • this run failed because there were diffs in the r-l/rust repo and the deno repo when testing the fn_call_layout changes before adding the Foo case.

  • this run passed because no diffs were detected when testing the fn_call_layout changes after adding the Foo case.

For anyone reading this that has no ideas what I'm talking about when I say Foo case, check out the discussion in #5337 (comment)

If you want to test the script manually you should be able to run the ci/check_diff.sh script directly and pass it the expected arguments.

For example:

ci/check_diff.sh https://github.com/ytmimi/rustfmt.git issue_5218

This new action is intended to help us maintainers determine when feature
branches cause breaking formatting changes by running rustfmt (master)
and the feature branch on various rust repositories.

Over time I expect the list of checked projects to increase.

With this action in place we can more easily test that a new feature or
bug fix doesn't introduce breaking changes. Although this action needs to
be manually triggered right now, we might consider adding it to our CI
runs in the future.
@ytmimi ytmimi force-pushed the diff_check_script branch from 68de8ea to 7399455 Compare August 3, 2022 00:16
Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

I've a few thoughts as we move forward, but view this as a rather significant value add and would rather go ahead and get it merged.

Thank you! Excited about this

@calebcartwright calebcartwright merged commit 949da52 into rust-lang:master Aug 13, 2022
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.

2 participants