DEPR: enforce a bunch #49552
DEPR: enforce a bunch #49552
Conversation
|
@MarcoGorelli pls advise on the pylint failures, not clear if they are actionable |
|
probably not, do you want to add Lines 60 to 78 in 1d8922e alongside this PR? |
Will this check start showing up in the pre-commit soon? Otherwise let's get rid of it. (and even if yes, im not a fan) |
|
yeah happy to get rid of it if you like - I'm glad that some of these pylint checks are helping find issue (like #49382 (review)), but others EDIT: will review the rest of the PR this afternoon |
doc/source/whatsnew/v2.0.0.rst
Outdated
| @@ -315,6 +317,7 @@ Removal of prior version deprecations/changes | |||
| - Removed argument ``line_terminator`` from :meth:`DataFrame.to_csv` and :meth:`Series.to_csv`, use ``lineterminator`` instead (:issue:`45302`) | |||
| - Removed argument ``inplace`` from :meth:`DataFrame.set_axis` and :meth:`Series.set_axis`, use ``obj = obj.set_axis(..., copy=False)`` instead (:issue:`48130`) | |||
| - Disallow passing positional arguments to :meth:`MultiIndex.set_levels` and :meth:`MultiIndex.set_codes` (:issue:`41485`) | |||
| - Disallow parsing to Timedelta strings with components with units "Y", "y", or "m", as these do not represent unambiguous durations (:issue:`47268`) | |||
There was a problem hiding this comment.
the issue number doesn't explain the context particularly well to users, is there a better reference? maybe #36838
There was a problem hiding this comment.
also, here you have 'm' whereas in the warning message there's 'M'
| compare( | ||
| ts_utc, | ||
| datetime.utcfromtimestamp(current_time), | ||
| ) | ||
|
|
||
| ts_utc = Timestamp.utcfromtimestamp(current_time) | ||
| assert ts_utc.timestamp() == current_time |
There was a problem hiding this comment.
just for my understanding, why did
compare(
ts_utc,
datetime.utcfromtimestamp(current_time),
)
have to change into
assert ts_utc.timestamp() == current_time
?
There was a problem hiding this comment.
because datetime.utcfromtimestamp(current_time) behaves differently from pd.Timestamp.utcfromtimestamp(current_time), so using compare to check they match would be wrong.
There was a problem hiding this comment.
not sure I follow, sorry, how would it be wrong?
the rest looks fine anyway, I've removed the 'requested changes', but I'll leave this change to someone else to approve
There was a problem hiding this comment.
Good point, the assertion would still hold. Either way is fine.
There was a problem hiding this comment.
IMO I am fine with not using compare here (especially with #49076 possibly changing .value)
mroeschke
added
Code Style
There was a problem hiding this comment.
Approving so as to remove my "requested changes", but I don't follow the change in #49552 (comment) so it'd be good if someone else could have a look at that
The rest looks good
|
Thanks @jbrockmendel |
* DEPR: enforce a bunch * docstring fixup * disable pylint check * pylint fixup * Fix gh ref in whatsnew
* DEPR: enforce a bunch * docstring fixup * disable pylint check * pylint fixup * Fix gh ref in whatsnew
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.