[SPARK-16779][TRIVIAL] Avoid using postfix operators where they do not add much and remove whitelisting #14407
Conversation
…g the result as a string we throw away
|
Oh, I thought the idea here was to un-whitelist most or all other occurrences. They're "whitelisted" just because they existed and were causing warnings, but most are probably not really good usages of postfix. At least I'd support a quick skim of the instances and undoing many usages of postfix. |
|
I'm totally down with removing the others - I'll go through and remove the whitelisting in the places where it doesn't add much. |
…aces cause it seems to probably improve readability there
|
Did a first pass audit, leaving XML parsing alone for now. |
|
Test build #63021 has finished for PR 14407 at commit
|
|
|
||
| killLeader() | ||
| delay(30 seconds) | ||
| delay(30.seconds) |
There was a problem hiding this comment.
Yeah, I'd like to hear opinions on this. "30 seconds" seems like one of the more reasonable usages of postfix. Is it worth keeping, or trying to get rid of cases like this?
There was a problem hiding this comment.
I think it might be reasonable to get rid of so that we can turn on the warnings to catch useful-ish things (like the "x isDefined")
There was a problem hiding this comment.
Personally I prefer 30 seconds. I counted 3 non-time-related usages in your change, so this feels like a lot of churn for not much gain - we could just fix those and remove the unnecessary imports where postfix ops are not used.
It'd also help if it were possible to turn these warnings into errors, since people don't generally pay attention to warnings...
There was a problem hiding this comment.
If the consesus is we want to keep the time based postfix operators then we can definitetly narrow this down (unfortunatetly those files will remain whitelisted as a whole and won't trigger any warnings so we won't necessarily notice if people start using other postfix operators inside of them).
There was a problem hiding this comment.
I think that's fine. Your patch shows that's really uncommon to begin with - most are caught during review.
There was a problem hiding this comment.
yah thats a good point we likely catch them during the codereview process so having the whitelisting probably isn't that bad. I'll switch this to leave the postfix time operators unless someone says otherwise over the weekend.
|
Test build #63026 has finished for PR 14407 at commit
|
|
Test build #63029 has finished for PR 14407 at commit
|
|
Test build #63040 has finished for PR 14407 at commit
|
holdenk
changed the title
holdenk
changed the title
|
Sounds like no objections on using postfix seconds operator - I'll go ahead and switch it to that. |
|
LGTM |
|
Test build #63373 has finished for PR 14407 at commit
|
|
Merging to master. |
4 participants
What changes were proposed in this pull request?
Avoid using postfix operation for command execution in SQLQuerySuite where it wasn't whitelisted and audit existing whitelistings removing postfix operators from most places. Some notable places where postfix operation remains is in the XML parsing & time units (seconds, millis, etc.) where it arguably can improve readability.
How was this patch tested?
Existing tests.