SPARK-6938: All require statements now have an informative error message. #5532
Conversation
|
ok to test |
|
Test build #30359 has finished for PR 5532 at commit
|
|
LGTM. CC @mengxr for any comments on the messages. |
| @@ -215,7 +215,8 @@ object Vectors { | |||
| require(prev < i, s"Found duplicate indices: $i.") | |||
| prev = i | |||
| } | |||
| require(prev < size) | |||
| require(prev < size, s"You may not write an element to index ${prev} because the declared " + | |||
There was a problem hiding this comment.
${prev} -> $prev, and same for ${size}, ${p}.
|
LGTM except minor style issues. |
|
I didn't see that in the spark scala style guide. It is probably worth updating there if you want people to stick with it. I tend to go with being 'strict' with ${} because of bad experiences with dropping the braces in bash. It isn't actualy needed here, just an anachronism. Would you prefer the change as additional commit on top of this, or for me to amend my previous commit and force push the single commit to my branch to replace the current one? |
|
@jhlch you can just add new commits to your branch (generally easier), when your changes are pushed they're all squashed into a single commit. |
|
I don't mind defensive braces as I don't know of a downside and also worry about this kind of thing in bash scripts. I don't think the code base does either consistently and I generally match nearby style. It's a toss-up here so I'd just make the extra commit. |
|
@vanzin That's cool. Is that done by the committer or something more magically automatic(jenkins?)? |
|
I think the scripts used by committers do the squashing. |
|
Cool, that makes more sense. Also, how convenient. I will leave this unsquashed. |
|
Test build #30380 has finished for PR 5532 at commit
|
|
Merged into master. Thanks! |
This pr adds informative error messages to all require statements in the Vectors class that did not previously have them. This references SPARK-6938.