Skip to content

Fix on wrong error raise in DataFrame.fillna#844

Merged
HyukjinKwon merged 1 commit into
databricks:masterfrom
charlesdong1991:error_raise_fillna
Sep 30, 2019
Merged

Fix on wrong error raise in DataFrame.fillna#844
HyukjinKwon merged 1 commit into
databricks:masterfrom
charlesdong1991:error_raise_fillna

Conversation

@charlesdong1991
Copy link
Copy Markdown
Contributor

if (value is None) and (method is None):

seems this line won't be touched, and the reason it still passed tests is because it triggered the Series.fillna() error raise mechanism in the else below.

@charlesdong1991 charlesdong1991 changed the title TST: Fix on wrong error raise in DataFrame.fillna Fix on wrong error raise in DataFrame.fillna Sep 29, 2019
@softagram-bot
Copy link
Copy Markdown

Softagram Impact Report for pull/844 (head commit: 0869897)

⭐ Change Overview

Showing the changed files, dependency changes and the impact - click for full size
(Open in Softagram Desktop for full details)

💡 Insights

  • Co-change Alert: You modified frame.py. Often test_dataframe.py (koalas/tests) is modified at the same time.

📄 Full report

Impact Report explained. Give feedback on this report to support@softagram.com

@codecov-io
Copy link
Copy Markdown

codecov-io commented Sep 29, 2019

Codecov Report

Merging #844 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #844   +/-   ##
=======================================
  Coverage   94.31%   94.31%           
=======================================
  Files          32       32           
  Lines        5930     5930           
=======================================
  Hits         5593     5593           
  Misses        337      337
Impacted Files Coverage Δ
databricks/koalas/frame.py 96.76% <100%> (+0.06%) ⬆️
databricks/koalas/series.py 95.07% <0%> (-0.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ee2a8d...0869897. Read the comment docs.

@HyukjinKwon
Copy link
Copy Markdown
Member

Thanks. Looks good but can we add a simple test @charlesdong1991?

@charlesdong1991
Copy link
Copy Markdown
Contributor Author

i think there is a test already there:

with self.assertRaisesRegex(ValueError,

but this triggered the error raising in Series.fillna. Do you still want a test? I could add one quickly @HyukjinKwon

@HyukjinKwon
Copy link
Copy Markdown
Member

Ah, gotya. LGTM

@HyukjinKwon HyukjinKwon merged commit aeccfb5 into databricks:master Sep 30, 2019
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