CLN: Break up wrap applied output #36536
Conversation
|
can you merge master and ill take a look |
…eakup_wrap_applied_output
|
@jbrockmendel Merged and passing thus far. Much of the diff is due to indentation being one level less, easier to view if you ignore whitespace. |
| keys, | ||
| values: List[Series], | ||
| not_indexed_same: bool, | ||
| first_not_none, |
There was a problem hiding this comment.
ideally if you can type the args (certainly can be a followon)
There was a problem hiding this comment.
I was thinking this too, but wasn't certain of types. We can't rely on mypy to catch improper typing here because this is being called from untyped code. I can guess at types and run tests with assert isinstance(var, guess) to confirm the guess, and rely on good test coverage. Is that an appropriate way to approach here?
There was a problem hiding this comment.
sure u can try that
ultimately we want to type all the way up and have mypy validate - but of course takes time and effort to get there
| keys, values, not_indexed_same=not_indexed_same | ||
| ) | ||
| # values are Series | ||
| return self._wrap_applied_output_series( |
There was a problem hiding this comment.
isn't L1198 and L1207 also a series?
There was a problem hiding this comment.
L1198 values is a list of either ndarrays or Index; L1207 values is a list of scalars.
There was a problem hiding this comment.
ok i c, can you make these clear in comments (followon ok).
Splitting off the Series case seemed to be a natural way to break up this method. Always open to other thoughts.