Skip to content

Conversation

@holdenk
Copy link
Contributor

@holdenk holdenk commented Feb 13, 2016

Some of the new doctests in ml/clustering.py have a lot of setup code, move the setup code to the general test init to keep the doctest more example-style looking.
In part this is a follow up to #10999
Note that the same pattern is followed in regression & recommendation - might as well clean up all three at the same time.

@SparkQA
Copy link

SparkQA commented Feb 13, 2016

Test build #51245 has finished for PR 11197 at commit bbd8287.

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

@holdenk
Copy link
Contributor Author

holdenk commented Feb 14, 2016

cc @mengxr who was interested in this cleanup

try:
(failure_count, test_count) = doctest.testmod(globs=globs, optionflags=doctest.ELLIPSIS)
sc.stop()
finally:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the try ... finally is not necessary because it does not handle any exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So finally is still useful even if we don't explicitly catch/handle any exceptions - are you saying the sc.stop and doctest will never throw any exceptions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for misunderstand, I think your are right.

@yanboliang
Copy link
Contributor

@holdenk I like this cleanup, thanks for the effort.

@yanboliang
Copy link
Contributor

LGTM

@holdenk
Copy link
Contributor Author

holdenk commented Feb 16, 2016

great :)

@holdenk
Copy link
Contributor Author

holdenk commented Feb 16, 2016

maybe @mengxr if you've got a chance to take a look at this?

@holdenk
Copy link
Contributor Author

holdenk commented Feb 17, 2016

or @davies or @jkbradley maybe?

@SparkQA
Copy link

SparkQA commented Feb 18, 2016

Test build #51486 has finished for PR 11197 at commit 885210f.

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

@srowen
Copy link
Member

srowen commented Feb 20, 2016

Merged to master

@asfgit asfgit closed this in 9ca79c1 Feb 20, 2016
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