-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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
[SPARK-3854] Scala style: require spaces before {
#2761
Conversation
Can one of the admins verify this patch? |
Jenkins, add to whitelist. This is ok to test. |
Test FAILed. |
Jenkins, retest this please. |
QA tests have started for PR 2761 at commit
|
QA tests have finished for PR 2761 at commit
|
Test FAILed. |
Oh, I didn't run scalastyle for yarn-alpha. |
QA tests have started for PR 2761 at commit
|
QA tests have finished for PR 2761 at commit
|
Test FAILed. |
QA tests have started for PR 2761 at commit
|
QA tests have finished for PR 2761 at commit
|
Test FAILed. |
QA tests have started for PR 2761 at commit
|
QA tests have finished for PR 2761 at commit
|
Test PASSed. |
I quite like standardizing style, but doesn't this have the same problem mentioned before, that it's going to break a lot of potential merge commits? If it's bite-the-bullet time, there are other micro changes that may actually have a little positive impact on execution that might be good to get in too. |
It would be neat if there was some way to restrict the style checker to only check new/changed lines introduced by a PR. This could be hard to integrate with local development workflows, though: I might be developing some code locally and periodically running scalastyle before opening a pull request, so we'd need to make sure that we don't emit tons of warnings from existing code. Maybe one approach would be to find all of the style warnings, then check whether the commits that introduced the lines that triggered the warnings are present in either |
When I did this for Python, I fixed all outstanding style problems as part of the same PR that introduced that check. It forced some people to rebase their open PRs, but it was a once-and-done thing. Are we opposed to doing that here? Trying to ease this check in by enforcing it only on new code is a good idea, but why not just get the style cleanup over with in this PR? It looks like @sarutak has done just that. Some people will have to rebase once and this style problem is done with. |
I also think, it's difficult to apply new style checker only to new codes. I cleaned up codes in origin/master for the style checker suggested in this PR. So, if this PR is merged, then we can enforce the new style to developers and all developers have to do is to check the style of the code changed by them. |
QA tests have started for PR 2761 at commit
|
QA tests have finished for PR 2761 at commit
|
Test PASSed. |
Hi @sarutak, We'd like to avoid making large refactorings for style, since these changes tend to create merge-conflicts when backporting to maintenance branches and make git blame significantly less useful. However, we'd be open to automatic style checks if they can be enforced only for new code (see https://issues.apache.org/jira/browse/SPARK-3849 for more details). In the meantime, do you mind closing this pull request? Thanks! |
O.K. I close this PR for now. |
This PR is a solution proposal of SPARK-3854.
Following is quoted from SPARK-3854:
We should require spaces before opening curly braces. This isn't in the style guide, but it probably should be:
See #1658 (diff) for an example "in the wild."
git grep "){" shows only a few occurrences of this style.