API: Rename CParserError to ParserError #14479

gfyoung · 2016-10-23T20:07:05Z

Title is self-explanatory. Partially resolves #12665.

codecov-io · 2016-10-23T22:37:16Z

Current coverage is 85.29% (diff: 66.66%)

Merging #14479 into master will increase coverage by <.01%

@@             master     #14479   diff @@
==========================================
  Files           140        140          
  Lines         50706      50709     +3   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43247      43250     +3   
  Misses         7459       7459          
  Partials          0          0

Powered by Codecov. Last update 748000d...dceed5c

jorisvandenbossche · 2016-10-24T09:21:08Z

I think we should keep the old name as an alias (or at least deprecate it)

jreback · 2016-10-24T10:32:05Z

agreed this needs a deprecation.

jreback · 2016-10-24T22:16:44Z

can you also run the tests and be sure that we are not generating this in the full test run (I mean the deprecation warning).

gfyoung · 2016-10-24T22:17:39Z

@jreback : I can run them locally again, but I'm pretty sure the answer is no. 😄

jorisvandenbossche · 2016-10-24T22:34:14Z

I would maybe go for a DeprecationWarning here in this case instead of FutureWarning, as this will be typically not used in interactive user code, but rather in library code (other option is to just keep it as alias)

gfyoung · 2016-10-24T23:52:45Z

@jorisvandenbossche : I'd rather deprecate since we would like to rename, though I don't quite see the necessity in making this one issue a DeprecationWarning instead of FutureWarning.

@jreback : Ran the tests locally and got no warnings about CParserError.

jorisvandenbossche · 2016-10-25T08:24:03Z

I'd rather deprecate since we would like to rename,

We did already rename it, my question was just whether to deprecate the old name or keep it as an alias, irregardless of the rename

though I don't quite see the necessity in making this one issue a DeprecationWarning instead of FutureWarning.

This will typically be used in library code. People using such a library will see the FutureWarning, but cannot do anything about it (the warning will only go away once the library was updated), which is annoying. With a deprecation warning you only see it on direct use.

gfyoung · 2016-10-25T12:39:09Z

People using such a library will see the FutureWarning, but cannot do anything about it (the warning will only go away once the library was updated), which is annoying. With a deprecation warning you only see it on direct use.

I fail to comprehend what you mean by that.

gfyoung · 2016-10-25T21:34:30Z

Putting [ci skip] in this commit until the failing master branch is patched.

gfyoung · 2016-10-26T13:37:32Z

@jreback , @jorisvandenbossche : Travis is now happy again. Ready to merge if there is nothing else.

jorisvandenbossche · 2016-10-26T13:56:38Z

People using such a library will see the FutureWarning, but cannot do anything about it (the warning will only go away once the library was updated), which is annoying. With a deprecation warning you only see it on direct use.

I fail to comprehend what you mean by that.

By default, a DeprecationWarning is not visible, a FutureWarning is. Therefore, we mostly use FutureWarnings for deprecations in pandas, as most things (eg changing keyword) are things all users need to be aware of. However, in this case most users will not directly use CParserError, but rather use a third party library that uses it (this is an assumption of me, not sure if this is correct). In such a case, a DeprecationWarning can be friendlier to users, as they otherwise see FutureWarnings about CParserError being deprecated that they have no control over, as it is used in the library they used. For that reason, we also used DeprecationWarning for the core.common deprecations (#13990)

jorisvandenbossche · 2016-10-26T13:58:13Z

Can you also add a test for the deprecated CParserError ?

gfyoung · 2016-10-26T15:41:35Z

@jorisvandenbossche : Ah, okay. Done. Let's see what Travis has to say now.

gfyoung · 2016-10-27T07:12:29Z

@jorisvandenbossche , @jreback : I changed the warning to be DeprecationWarning, and Travis seems to be happy. Ready to merge if there are no other concerns.

jorisvandenbossche · 2016-10-27T07:16:43Z

pandas/io/tests/test_common.py

+        with tm.assert_produces_warning(DeprecationWarning,
+                                        check_stacklevel=False):
+            with tm.assertRaises(common.ParserError):
+                raise common.CParserError()


A common usage will also be:

from pandas.io.common import CParserError try: .... except CParserError: ....

but I don't think this will currently warn for such use cases?

And some usages I found seem to also import this from pandas.parser, so maybe we should keep it there as well (so not only in io.common).

https://github.com/glue-viz/glue/blob/e9742059112d00e562fc3123703678d66fad1996/glue/core/data_factories/pandas.py#L60
https://github.com/sawadyrr5/pyjsf/blob/cf1e8643afb65912237483eac5a2735df5a8a8a6/pyjsf/data/download.py

@jorisvandenbossche: Actually, I get an error if you do that:

try: ... except CParserError: ... TypeError: catching classes that do not inherit from BaseException is not allowed

That's probably because of the decorator usage? (as it works for master)

Yes, it is because of the decorator usage. So at least that prevents try-except.

Well I would sure hope it works on master!

gfyoung · 2016-11-10T05:03:44Z

Getting some weird segfaults that seem unrelated to this PR (also in my other one)...disabling testing for the time being.

gfyoung · 2016-11-13T20:05:05Z

@jorisvandenbossche , @jreback : No more segfaulting, and everything looks green. Ready to merge if there are no concerns.

jorisvandenbossche · 2016-11-14T09:58:43Z

I would first like to get agreement on the status of CParserError: do we regard it as an alias (which means for me keeping it), or as deprecated and with the intention to remove it?

I asked SO if it possible to deprecate an Exception (http://stackoverflow.com/questions/40545005/is-it-possible-to-deprecate-an-exception-class/40546615#40546615), but there did not really appear a nice solution

gfyoung · 2016-11-17T17:26:58Z

@jorisvandenbossche : The intention is to remove CParserError as both I and @jreback have said (see here for his comment regarding this). However, if there is no nice way to deprecate an exception, then aliasing as a preliminary step is the only way to go at this point.

jorisvandenbossche · 2016-11-17T23:38:46Z

pandas/parser.pyx

-from io.common import CParserError, DtypeWarning, EmptyDataError
+# XXX: It's annoying that we have to import both CParserError
+# and ParserError. Unfortunately, backwards compatibility is
+# the higher calling right now.


Can you keep this a bit more 'dry' and simply say "import CParserError as alias of ParserError for backward compatibility"

you can maybe also put the one import of CParserError on a separate line to make this more clear

Sounds good. Done.

jorisvandenbossche

Can you also add a test for the alias? (you had one before I think)

jorisvandenbossche · 2016-11-17T23:40:45Z

doc/source/whatsnew/v0.20.0.txt

@@ -41,6 +41,7 @@ Backwards incompatible API changes
 .. _whatsnew_0200.api:


+- ``CParserError`` has been aliased to ``ParserError`` in ``pd.read_csv`` and will be removed in the future (:issue:`12665`)


Maybe say "renamed to" ? For me that sounds stronger that I have to change my code if I use it

Sounds good. Done.

Partially resolves pandas-devgh-12665. We will remove CParserError in the future.

gfyoung · 2016-11-18T03:17:43Z

@jorisvandenbossche : Added a slightly different alias test, but yes, done.

jorisvandenbossche · 2016-11-18T13:38:45Z

@gfyoung Thanks! (just added the same test for pd.parser.CParserError)

gfyoung force-pushed the csv-error-consistent branch from cb53574 to 19f626a Compare October 23, 2016 20:13

jorisvandenbossche added Error Reporting Incorrect or improved errors from pandas IO CSV read_csv, to_csv labels Oct 24, 2016

gfyoung force-pushed the csv-error-consistent branch from 19f626a to 7d3ddd3 Compare October 24, 2016 15:18

gfyoung changed the title ~~API: Rename CParserError to ParserError.~~ DEPR: Deprecate CParserError in favor of ParserError Oct 24, 2016

gfyoung force-pushed the csv-error-consistent branch 2 times, most recently from 07ef63d to c7d00f1 Compare October 24, 2016 22:15

gfyoung force-pushed the csv-error-consistent branch from c7d00f1 to ed1fa3e Compare October 24, 2016 23:54

jorisvandenbossche added this to the 0.20.0 milestone Oct 25, 2016

gfyoung force-pushed the csv-error-consistent branch 2 times, most recently from 8cbafe4 to 3d2619e Compare October 25, 2016 21:34

gfyoung force-pushed the csv-error-consistent branch from 3d2619e to 0f4e4aa Compare October 26, 2016 00:12

gfyoung force-pushed the csv-error-consistent branch from 0f4e4aa to 1f5e769 Compare October 26, 2016 15:41

gfyoung force-pushed the csv-error-consistent branch from 1f5e769 to 542f398 Compare October 27, 2016 00:31

jorisvandenbossche reviewed Oct 27, 2016

View reviewed changes

gfyoung changed the title ~~DEPR: Deprecate CParserError in favor of ParserError~~ DEPR: Alias CParserError to ParserError Nov 10, 2016

gfyoung changed the title ~~DEPR: Alias CParserError to ParserError~~ API: Alias CParserError to ParserError Nov 10, 2016

gfyoung force-pushed the csv-error-consistent branch 8 times, most recently from d83c8df to 8d7f1b5 Compare November 13, 2016 07:43

gfyoung force-pushed the csv-error-consistent branch 4 times, most recently from 3b7bee6 to 41cc041 Compare November 17, 2016 17:04

jorisvandenbossche reviewed Nov 17, 2016

View reviewed changes

gfyoung changed the title ~~API: Alias CParserError to ParserError~~ API: Rename CParserError to ParserError Nov 18, 2016

API: Rename CParserError to ParserError.

912668e

Partially resolves pandas-devgh-12665. We will remove CParserError in the future.

gfyoung force-pushed the csv-error-consistent branch from 41cc041 to 912668e Compare November 18, 2016 03:16

add test for parser.CParserError

dceed5c

jorisvandenbossche merged commit c045e1d into pandas-dev:master Nov 18, 2016

gfyoung deleted the csv-error-consistent branch November 18, 2016 14:33

jorisvandenbossche mentioned this pull request Jan 11, 2017

DEPR: relocate exceptions in pandas.core.common #14800

Closed

jorisvandenbossche mentioned this pull request Jan 20, 2017

Relocate exceptions #14800 #15181

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: Rename CParserError to ParserError #14479

API: Rename CParserError to ParserError #14479

gfyoung commented Oct 23, 2016 •

edited

Loading

codecov-io commented Oct 23, 2016 •

edited

Loading

jorisvandenbossche commented Oct 24, 2016

jreback commented Oct 24, 2016

jreback commented Oct 24, 2016

gfyoung commented Oct 24, 2016

jorisvandenbossche commented Oct 24, 2016

gfyoung commented Oct 24, 2016 •

edited

Loading

jorisvandenbossche commented Oct 25, 2016

gfyoung commented Oct 25, 2016

gfyoung commented Oct 25, 2016

gfyoung commented Oct 26, 2016

jorisvandenbossche commented Oct 26, 2016 •

edited

Loading

jorisvandenbossche commented Oct 26, 2016

gfyoung commented Oct 26, 2016

gfyoung commented Oct 27, 2016

jorisvandenbossche Oct 27, 2016

jorisvandenbossche Oct 27, 2016

gfyoung Oct 27, 2016

jorisvandenbossche Oct 27, 2016

gfyoung Oct 27, 2016

gfyoung commented Nov 10, 2016

gfyoung commented Nov 13, 2016

jorisvandenbossche commented Nov 14, 2016

gfyoung commented Nov 17, 2016 •

edited

Loading

jorisvandenbossche Nov 17, 2016

gfyoung Nov 18, 2016

jorisvandenbossche left a comment

jorisvandenbossche Nov 17, 2016

gfyoung Nov 18, 2016

gfyoung commented Nov 18, 2016

jorisvandenbossche commented Nov 18, 2016

		@@ -41,6 +41,7 @@ Backwards incompatible API changes
		.. _whatsnew_0200.api:


		- ``CParserError`` has been aliased to ``ParserError`` in ``pd.read_csv`` and will be removed in the future (:issue:`12665`)

API: Rename CParserError to ParserError #14479

API: Rename CParserError to ParserError #14479

Conversation

gfyoung commented Oct 23, 2016 • edited Loading

codecov-io commented Oct 23, 2016 • edited Loading

Current coverage is 85.29% (diff: 66.66%)

jorisvandenbossche commented Oct 24, 2016

jreback commented Oct 24, 2016

jreback commented Oct 24, 2016

gfyoung commented Oct 24, 2016

jorisvandenbossche commented Oct 24, 2016

gfyoung commented Oct 24, 2016 • edited Loading

jorisvandenbossche commented Oct 25, 2016

gfyoung commented Oct 25, 2016

gfyoung commented Oct 25, 2016

gfyoung commented Oct 26, 2016

jorisvandenbossche commented Oct 26, 2016 • edited Loading

jorisvandenbossche commented Oct 26, 2016

gfyoung commented Oct 26, 2016

gfyoung commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfyoung commented Nov 10, 2016

gfyoung commented Nov 13, 2016

jorisvandenbossche commented Nov 14, 2016

gfyoung commented Nov 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfyoung commented Nov 18, 2016

jorisvandenbossche commented Nov 18, 2016

gfyoung commented Oct 23, 2016 •

edited

Loading

codecov-io commented Oct 23, 2016 •

edited

Loading

gfyoung commented Oct 24, 2016 •

edited

Loading

jorisvandenbossche commented Oct 26, 2016 •

edited

Loading

gfyoung commented Nov 17, 2016 •

edited

Loading