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

Fix/log incorrect kwarg #236

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

martialo12
Copy link

Incorrect kwarg is not logged or rejected

Hi,
I've come across an issue detailed in GitHub Issue #212 regarding the Meraki Dashboard API where incorrect kwargs are neither logged nor rejected. I believe I have developed a solution that could effectively address this problem.

My approach involves adding a validation mechanism to ensure that only supported kwargs are accepted by the API functions. This would involve a function that cross-checks each kwarg against a predefined list of accepted parameters, raising an error or logging a warning for any unsupported kwargs.

I think this solution could significantly enhance the robustness of the API by preventing silent failures and providing clearer feedback to users. If this approach proves helpful, I am also ready to implement a similar solution for the aio (asynchronous) API service.

I am looking forward to your feedback on this suggestion and am happy to discuss further details or adjustments as needed

boxdev and others added 22 commits December 13, 2023 18:58
@TKIPisalegacycipher
Copy link
Collaborator

TKIPisalegacycipher commented Dec 14, 2023

Hi @martialo12 thank you for tackling #212 and for this PR!

The library is generated dynamically via the generate_library.py file, so for these changes to stick, the jinja templates will need to be updated as well. Without updating the jinja templates to include the new validate_kwargs function, the next generation of the library will wipe out your changes.

The jinja templates are here and I don't think it'd take much effort, but I don't think I'll be able to do it myself before the end of the year. Would you like to make the change and submit a second PR?

Separately, it seems like this validation is on by default, but for backwards compatibility purposes, it should be off by default. So adding an option to config.py and a kwarg to the base classes where users can optionally turn on this validation would be a requirement. Please do consider including such a change in your PR.

And thanks again for your great work!

@martialo12
Copy link
Author

Hi @martialo12 thank you for tackling #212 and for this PR!

The library is generated dynamically via the generate_library.py file, so for these changes to stick, the jinja templates will need to be updated as well. Without updating the jinja templates to include the new validate_kwargs function, the next generation of the library will wipe out your changes.

The jinja templates are here and I don't think it'd take much effort, but I don't think I'll be able to do it myself before the end of the year. Would you like to make the change and submit a second PR?

Separately, it seems like this validation is on by default, but for backwards compatibility purposes, it should be off by default. So adding an option to config.py and a kwarg to the base classes where users can optionally turn on this validation would be a requirement. Please do consider including such a change in your PR.

And thanks again for your great work!

I really appreciate your feedback. This is the new PR(#238) according to your suggestions.

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.

2 participants