Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to ignore invalid ids #23

Merged
merged 1 commit into from Feb 21, 2020

Conversation

mbkupfer
Copy link
Contributor

@OliverSherouse, I think it would be a vast improvement if we can add the option to ignore invalid ids. There are times where I have just one or two invalid series ids in the mix, but the rest are fine. I don't see why the whole get_series request should fail because of those couple wrong ids.

The new approach still let's the user know if there was a bad id (or ids), but it instead throws a warning message instead. This all requires using a new keyword argument though, called errors. I modeled this off of pandas.Dataframe.drop which also has the same type of argument.

Besides that change, I also had to adjust a few of the test cases since a dataframe is now being returned instead of a series. Reason is because I'm now using an array to create the final returned dataframe and that coerces the series into a dataframe. In the end, I actually think this is an improvement since the datatype doesn't change from the enduser perspective.

@mbkupfer
Copy link
Contributor Author

@OliverSherouse have you had a chance to review?

@OliverSherouse OliverSherouse changed the base branch from dev to invalid-ids February 20, 2020 02:48
@OliverSherouse
Copy link
Owner

I know it's been a good while, but can I ask for you to rebase this off the current branch? If you're no longer interested just let me know and I'll integrate these changes myself.

@mbkupfer
Copy link
Contributor Author

Yeah no problem, I can rebase. Thanks for getting back on this.

@OliverSherouse OliverSherouse changed the base branch from invalid-ids to dev February 20, 2020 18:09
Added a new keyword argument for get_series which allows for invalid ids
to simply be ignored rather then fail. This will avoid unecessary
requests for series that were valid, but happened to be grouped with a
invalid series.

Had to also rewrite a few of the tests since this new version returns a
dataframe instead of series when one one series is requests.
@mbkupfer
Copy link
Contributor Author

@OliverSherouse, I resolved all conflicts and re-pushed. Checks are failing though, but I believe it is unrelated. Either way, let me know if you have any thoughts or questions.

@OliverSherouse OliverSherouse changed the base branch from dev to invalid-ids February 21, 2020 19:29
@OliverSherouse OliverSherouse merged commit c12c52e into OliverSherouse:invalid-ids Feb 21, 2020
@OliverSherouse
Copy link
Owner

Thanks! I'm also a bit unclear on what's happening with that test, so I'm going to redirect into an invalid-ids branch and solve that, as well as make a few changes to keep the current behavior to return a pd.Series if only one argument is given as series. Thanks so much for your contribution!

OliverSherouse added a commit that referenced this pull request Feb 22, 2020
* Add ability to ignore invalid ids (#23)

Added a new keyword argument for get_series which allows for invalid ids
to simply be ignored rather then fail. This will avoid unnecessary
requests for series that were valid, but happened to be grouped with a
invalid series.

Fix some typing issues.

Closes #30 

Co-authored-by: Maxim Kupfer <mbkupfer@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants