Skip to content

TST replace assert_raise_* by pytest.raises in neighbors module#19388

Merged
glemaitre merged 6 commits intoscikit-learn:mainfrom
Haidar13:fixassert
Feb 22, 2021
Merged

TST replace assert_raise_* by pytest.raises in neighbors module#19388
glemaitre merged 6 commits intoscikit-learn:mainfrom
Haidar13:fixassert

Conversation

@Haidar13
Copy link
Copy Markdown
Contributor

@Haidar13 Haidar13 commented Feb 6, 2021 •
edited
Loading

Reference Issues/PRs

This PR is related to issue #14216

What does this implement/fix? Explain your changes.

Change assert_raises and assert_raises_regex to :

with pytest.raises(.....)

@Haidar13 @stevenkolawole
Change assert_raises into pytest.raises in sklearn/neighbors/test
d6702a2 
Co-authored-by: SteveKola <kolawolesteven99@gmail.com>
@reshamas
Copy link
Copy Markdown
Member

reshamas commented Feb 6, 2021

#DataUmbrella sprint
cc: @Mariam-ke

@glemaitre glemaitre self-requested a review February 7, 2021 09:49
glemaitre
glemaitre reviewed Feb 7, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

Could you in addition check the following file to replace assert_raise_message by the the pytest.raises context manager as well:

  • test_optics.py
  • test_mean_shift.py
  • test_hierarchical.py

@glemaitre glemaitre changed the title [MRG] Change assert_raises to pytest.raises in sklearn/neighbors/test TST replace assert_raise_* by pytest.raises in neighbors module Feb 7, 2021
@Haidar13
Copy link
Copy Markdown
Contributor Author

Haidar13 commented Feb 13, 2021

@glemaitre all raise_message and raise_warn have been changed to the with pytest context

@reshamas
Copy link
Copy Markdown
Member

reshamas commented Feb 15, 2021

cc: @SteveKola

@Haidar13 Would you please remove Steven's email address from the PR description at the top? I tagged his GitHub handle here. Thank you.

@reshamas reshamas mentioned this pull request Feb 15, 2021
Closed
@Haidar13
Copy link
Copy Markdown
Contributor Author

Haidar13 commented Feb 15, 2021

cc: @SteveKola

@Haidar13 Would you please remove Steven's email address from the PR description at the top? I tagged his GitHub handle here. Thank you.

I removed the email address,
Thanks

ogrisel
ogrisel reviewed Feb 18, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

I introduced spurious changes when resolving the conflicts when merging the main branch. Let me undo those.

ogrisel
ogrisel reviewed Feb 18, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Minor style comment. Otherwise, looks good to me.

ogrisel
ogrisel approved these changes Feb 20, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR!

@Haidar13
Copy link
Copy Markdown
Contributor Author

Haidar13 commented Feb 20, 2021

@ogrisel
@glemaitre

Thank you for spending time reviewing my PR.
Looking forward to contributing more

@glemaitre glemaitre merged commit c3eb5ed into scikit-learn:main Feb 22, 2021
@glemaitre
Copy link
Copy Markdown
Member

glemaitre commented Feb 22, 2021

Thanks @Haidar13 It looks good.

@lorentzenchr lorentzenchr mentioned this pull request Mar 2, 2021
Closed
43 tasks
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@glemaitre glemaitre glemaitre left review comments

@ogrisel ogrisel ogrisel approved these changes

Assignees

No one assigned

Labels

module:neighbors Waiting for Reviewer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Haidar13 @reshamas @glemaitre @ogrisel