-
Notifications
You must be signed in to change notification settings - Fork 154
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
Incorrect kwarg is not logged or rejected #212
Comments
Thanks for reporting this @mpenning ! Adding additional error conditions to the library methods could be seen as a breaking change; scripts otherwise working would stop working and this could be a big problem for folks who are not actively maintaining their scripts. I don't think this is a security concern so it's harder to justify in that sense. Did you have a suggested approach for handling this? Especially if you have a PR, that'd be helpful. |
Today, errors pass silently... and the result is VERY bad... the API returns partial results. That broke my script, but only because I knew exactly how many networks to expect. Imagine if I didn't catch the problem and it broke my production network. Your claim is essentially "rejecting an invalid keyword could break a running script". The Zen of Python is very clear about this... "Errors should never pass silently... Unless explicitly silenced." Logging a red error message to stdout seems rather unlikely to be a breaking change. Perhaps we can agree on this point. |
Adding an entry to the logger with that severity on such a scenario sounds like a good enhancement. Would you like to send a PR making that change? |
Sounds good... it may take a while, I'm not familiar with the inner working of things |
I spent a while diagnosing why my calls to
getOrganizationNetworks()
did not return ~20% of the networks that I expected to see.My call was:
getOrganizationNetworks(organizationId="000000000000000000", direction= "prev", total_pages= "all", per_page=100000)
I got back about 8500 networks; I expected about 10750.
At first, I thought it might be a concurrency problem (I use the meraki api with
mpire
), but the concurrency investigation was a dead-end.Short story, one of my parameter names is incorrect.
per_page
should beperPage
. It doesn't help that somegetOrganizationNetworks()
API keywords are snake case, and others are camel case. After I fixed the problem, I got all the networks that I expected. However, I don't think this should happen. If I call the Meraki API an unsupportedkwarg
, you should throw a warning or error.The text was updated successfully, but these errors were encountered: