search

meraki / dashboard-api-python

Official Dashboard API library (SDK) for Python
MIT License
287 stars 149 forks source link

Fix/log incorrect kwarg #236

Open martialo12 opened 8 months ago

martialo12 commented 8 months ago

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

TKIPisalegacycipher commented 8 months ago

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 commented 8 months ago

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(https://github.com/meraki/dashboard-api-python/pull/238) according to your suggestions.