BUG: Concat typing

[rhshadrach]

• closes #xxxx
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

cc @simonjayhawkins

Attempting to add typing to results in aggregation.transform. Here, the obj can be a Series but the list passed into concat contain frames. mypy correctly identifies this as an issue, I think concat needs to be typed as FrameOrSeriesUnion for this purpose.

When I try just typing the function definition and not the overload, mypy complains:

error: Overloaded function implementation does not accept all possible arguments of signature 2  [misc]

so I've changed the type of the overload to FrameOrSeriesUnion as well. I don't believe this is any weaker than the current state because it is an overload.

[rhshadrach]

mypy fails in the CI check here because concat is called within NDFrame, which is not included in FrameOrSeriesUnion. I think part of the issue might be some odd behavior I'm seeing with Mapping and mypy. Still working through it, marking as a draft.

[simonjayhawkins]

@rhshadrach #30852 (review) might help with the backstory for this.

It is worth noting that FrameOrSeriesUnion reports errors if concat called from an abstract/base class. And FrameOrSeries is a typevar and hence is used to preserve types. Maybe just NDFrame is what you require?

[rhshadrach]

Thanks @simonjayhawkins, I've updated the type-hints to use NDFrame. We don't want FrameOrSeriesUnion here because concat can be called from NDFrame and we don't want FrameOrSeries because the argument could be a list of both frames and series.

One other thing I'll note is that while trying to provide various overloads, I ran into this bug that seems to impact many uses of overload where the arguments are Mapping or Dict (among other things).

[WillAyd]

It is worth noting that FrameOrSeriesUnion reports errors if concat called from an abstract/base class

Unrelated to this PR but what is the point of FrameOrSeriesUnion vs just using NDFrame? Just more user-friendly from an API perspective right?

[simonjayhawkins]

It is worth noting that FrameOrSeriesUnion reports errors if concat called from an abstract/base class

Unrelated to this PR but what is the point of FrameOrSeriesUnion vs just using NDFrame? Just more user-friendly from an API perspective right?

@WillAyd the naming was decided in #30582. Looking back through the PR I think both of us prefered a FrameOrSeries and FrameOrSeriesT naming convention. That's also reason I closed #36100. I didn't want to propagate the current naming convention. #36100 (comment)

and I think somewhere in the history before that it was prefered to not have NDFrame in type annotations that might be visible to end users.

I'm +1 on NDFrame and NDFrameT where more appropriate.

[WillAyd]

lgtm

[jreback]

didn't we decide that NDFrame is confusing?

so am confused by

@WillAyd the naming was decided in #30582. Looking back through the PR I think both of us prefered a FrameOrSeries and FrameOrSeriesT naming convention. That's also reason I closed #36100. I didn't want to propagate the current naming convention. #36100 (comment)

@WillAyd you are now +1 on this?

[WillAyd]

Within the scope of this PR I think it make sense to not try and use the Union type of two objects in the parent.

Probably fine to have that Union from an API perspective but doesn't need to be forced internally

[jreback]

ok then,

should clarify in _typing on what should be used when for these types.