Converts statcast over to using the try_parse code that autodetermines data types #106
Conversation
Fixed very stupid typo in README
Merge master in
Team Batting (FanGraphs) => Add unit tests; use new consolidated data…
team_pitching to new FanGraphs datasource (jldbc#96)
| '%Y-%m-%dT%H:%M:%S.%fZ', # Just in case (https://github.com/jldbc/pybaseball/issues/104) | ||
| ] | ||
|
|
||
| def try_parse_dataframe( |
There was a problem hiding this comment.
Can we make this fit on one line? Maybe by setting something for Unon[str, int, float', datetime']?
There was a problem hiding this comment.
Define ColType = Union[str, int, float', datetime'] somewhere, and annotate ColType?
There was a problem hiding this comment.
Do you find it less readable on multiple lines? I'll put it on one line if desired, but I find it more readable this way. You near instantly can see how many parameters it takes, of what type, and which have defaults. Whereas one one line they tend to somewhat run together in my opinion.
Thoughts?
There was a problem hiding this comment.
Also digging through the PEP to see what are the preferred way to handle this and found this discussion:
Where they suggested an alternative that would look like this if you'd prefer (though I personally find it less readable):
def try_parse_dataframe(data: pd.DataFrame, null_replacement: Union[str, int, float, datetime] = np.nan,
known_percentages: List[str] = []) -> pd.DataFrame:
|
LGTM |
…s data types (#106) * Add unit tests for statcast to ensure backwards compatibility * Convert statcast to using try_parse * Add some missing testing
This PR was done in three steps:
When merged this PR should fix the following issues: #105, #104, #59