-
Notifications
You must be signed in to change notification settings - Fork 737
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
Add a newline between assertThrows and the following assertions: #1084
Add a newline between assertThrows and the following assertions: #1084
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
Add a newline after assertThrows with an expression lambda and the following assertions.
CLAs look good, thanks! |
We usually run a formatter (e.g.) on any modified lines after running refactorings, instead of trying to make sure the suggested fixes are well formatted. Is that an option for you?
By default it does an AST diff and ignores whitespace, but there's an example of a whitespace-sensitive test here: error-prone/core/src/test/java/com/google/errorprone/bugpatterns/MultiVariableDeclarationTest.java Line 63 in 2623f22
|
I see, that makes sense! I certainly do not propose to properly indent the code, but having two statements on a single line results in quite a long line in a patch. Auto-formatting might not work if a user has a build configuration that checks that that the formatting is valid, but does not enforce a canonical formatting. In this case running a formatter that cannot operate on changed lines only might result in unrelated changes.
Thank you for the reference! Do you think it is worth including a whitespace-sensitive test in this case or would it be too fragile? I've attached a patch, if it's OK, I would push it to this branch. positiveMatchingExceptionMessage test
|
Thanks, I think skipping the test is OK in this case. I'll merge this change momentarily. |
Overview
Add a newline after assertThrows with an expression lambda and the following assertions,
so that each statement appears on its own line.
Note
com.google.errorprone.BugCheckerRefactoringTestHelper#addOutputLines
does not seem to care about any whitespaces, therefore, I couldn't include a failing test.