-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
API/DEPR: replace "raise_conflict" with "errors" for df.update #23657
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hello @h-vetinari! Thanks for updating the PR.
Comment last updated on November 14, 2018 at 20:44 Hours UTC |
warnings.simplefilter("ignore", FutureWarning) | ||
# do not warn about constructing Panel when reindexing | ||
result = super(Panel, self).reindex(**kwargs) | ||
return result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied from #23192:
I had no intention to add this warning filter here, but otherwise I cannot test the
raise_conflict
deprecation forPanel
, as this warning was throwing me off from trying to catch the deprecation warning. If we don't care about testing that kwarg deprecation forPanel
, I can revert this change here.
Panel, or object coercible to Panel. Aligns on items | ||
Modify Panel in place using non-NA values from other Panel. | ||
|
||
May also use object coercible to Panel. Will align on items. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran the doctest validation, therefore changed more than just the errors
text. But not essential to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is fine
Returns | ||
------- | ||
None : method directly changes calling object | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added due to running docstring validation
Codecov Report
@@ Coverage Diff @@
## master #23657 +/- ##
==========================================
+ Coverage 92.24% 92.25% +<.01%
==========================================
Files 161 161
Lines 51339 51346 +7
==========================================
+ Hits 47360 47367 +7
Misses 3979 3979
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @jreback
cc @datapythonista (b/c of the doc changes)
raise_conflict : bool, default False | ||
If True, will raise a ValueError if the DataFrame and `other` | ||
errors : {'raise', 'ignore'}, default 'ignore' | ||
If 'raise', will raise a ValueError if the DataFrame and `other` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
versionchanged tag
Returns | ||
------- | ||
None : method directly changes calling object | ||
|
||
Raises | ||
------ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add an if for valid errors values (e.g. raise/error), and test for passing bad values in the tests themselves
Panel, or object coercible to Panel. Aligns on items | ||
Modify Panel in place using non-NA values from other Panel. | ||
|
||
May also use object coercible to Panel. Will align on items. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is fine
@@ -322,7 +322,9 @@ def test_update_raise(self): | |||
other = DataFrame([[2., nan], | |||
[nan, 7]], index=[1, 3], columns=[1, 2]) | |||
with pytest.raises(ValueError, match="Data overlaps"): | |||
df.update(other, raise_conflict=True) | |||
df.update(other, errors='raise') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you move this to another test, e.g. test_update_deprecation
thanks |
* upstream/master: BUG: to_html misses truncation indicators (...) when index=False (pandas-dev#22786) API/DEPR: replace "raise_conflict" with "errors" for df.update (pandas-dev#23657) BUG: Append DataFrame to Series with dateutil timezone (pandas-dev#23685) CLN/CI: Catch that stderr-warning! (pandas-dev#23706) ENH: Allow for join between two multi-index dataframe instances (pandas-dev#20356) Ensure Index._data is an ndarray (pandas-dev#23628) DOC: flake8-per-pr for windows users (pandas-dev#23707) DOC: Handle exceptions when computing contributors. (pandas-dev#23714) DOC: Validate space before colon docstring parameters pandas-dev#23483 (pandas-dev#23506) BUG-22984 Fix truncation of DataFrame representations (pandas-dev#22987)
git diff upstream/master -u -- "*.py" | flake8 --diff
This is split off from #23192, as requested per review (because the PR was getting larger and larger).