Skip to content

Conversation

@holdenk
Copy link
Contributor

@holdenk holdenk commented Sep 2, 2015

Params.getOrDefault should throw a more meaningful exception than what you get from a bad key lookup.

Copy link
Member

Choose a reason for hiding this comment

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

Can you check if the map contains the param rather than wait for an exception? Really doesn't matter though. This looks OK except that you'll need a space after "param" in the error message below, and you could use string interpolation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured it was better to handle the exception case this way since the exception case should happen way less frequently. I'll switch it to string interpolation tomorrow :)

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have performance issues here. So code readability is more important, e.g.,

defaultParamMap.getOrElse(param, 
  throw new NoSuchElementException(s"No default value for ${param.name} in ${this.getClass}.")

@SparkQA
Copy link

SparkQA commented Sep 2, 2015

Test build #41918 has finished for PR 8567 at commit 2f6c14d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Sep 2, 2015

Test build #41945 has finished for PR 8567 at commit 1b502b9.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Sep 2, 2015

Test build #41949 has finished for PR 8567 at commit 0235a6a.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Sep 2, 2015

Test build #41950 has finished for PR 8567 at commit bf324c4.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@asfgit asfgit closed this in 44948a2 Sep 3, 2015
@mengxr
Copy link
Contributor

mengxr commented Sep 3, 2015

LGTM. Merged into master. Thanks!

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.

4 participants