Fix 25648 improve read csv stacktrace on bad argument

[roberthdevries]

• closes ERR: read_csv exposes an internal function when bad argument is specified #25648
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

[roberthdevries]

It works, but it may not be pretty because it uses exec() which is apparently an unwanted pattern

[WillAyd]

Yea this is pretty hacky :-X is this really the only option?

[gfyoung]

-1 on this change unfortunately. exec is extremely hacky for something that is arguably minor.

While I don't have an immediate alternative off the top of my head, I think it's better to let this one percolate a little than to force a fix.

[roberthdevries]

The options to fix this are limited by the fact that the exception raising mechanism uses a read-only variable of the code attribute of a function: <func>.code.co_name.
This variable is set by the python compiler/interpreter by taking the name of the function when it is defined: def <func>. Which means that we have to do def read_csv() and def read_table() to fix this issue.

I see 4 options in total:

1. Use exec() to dynamically create the functions with the appropriate names
2. Make two duplicates of parser_f with all of its arguments, factor out the body of the function
3. Make a template function e.g. parser_f.py.in and generate read_csv.py and read_table.py at build time
4. Don't fix this issue 😃

May I have your suggestions/votes please

[gfyoung]

1. -1: too hacky as already discussed

2. -1: maintenance burden > error correctness

3. -0.5: less maintenance burden, but I still feel creating a template just for this is too much

4. -0: It's worked us for awhile, but I don't want to give up on this just yet.

Middle ground option:

Rename parser_f to something like read_document ? Then the function name isn't as foreign sounding and at least aligns with what read_csv and read_table do.

Another middle ground option:

Create a wrapper around parser_f than takes in *args and **kwargs and catch TypeError to replace instances of the string parser_f with the function name.

The disadvantage is we lose the signature when we do help on these functions.

[jreback]

agree with comments here - we cannot use string execution like this

you an try setting

parser_f.name = ...

[gfyoung]

agree with comments here - we cannot use string execution like this

you an try setting

parser_f.name = ...

Unfortunately, the stacktrace uses a read-only attribute to provide the function name in that error.

[jbrockmendel]

@roberthdevries can you rebase

[gfyoung]

@roberthdevries can you rebase

Before rebasing, I think it's only fair that we get some consensus on how to address the underlying problem, given that we largely agree that the proposed fix is not something we would like to merge.

[roberthdevries]

I am currently ill, and not able to work on this

[gfyoung]

I am currently ill, and not able to work on this

@roberthdevries : Oh no! Very sorry to hear that. Hope you get better soon.

We can take over this PR once we get some consensus.

[jbrockmendel]

is it possible compat.set_function_name may work where just setting func.__name__ doesnt?

[gfyoung]

is it possible compat.set_function_name may work where just setting func.__name__ doesnt?

@jbrockmendel : Unfortunately not. As mentioned in the OP, the function name in the error message is populated from a read-only attribute.

[jbrockmendel]

OK, then im inclined towards the emerging consensus of "learn to live with it"

[gfyoung]

OK, then im inclined towards the emerging consensus of "learn to live with it"

Learn to live with it? We've been living with this stacktrace issue for years 😄

[simonjayhawkins]

@roberthdevries Thanks for looking into this. closing for now as stale. ping if you want to continue.

[roberthdevries]

Can you mark #25648 as wontfix?

[simonjayhawkins]

added a needs discussion label and removed the contributions welcome for now.