4doom4 / python-voipms

Python client for v1 of voip.ms REST API
MIT License
23 stars 11 forks source link

Reserved word as keyword argument #17

Closed JohnMertz closed 3 months ago

JohnMertz commented 4 months ago

This is probably a very newb question, so I apologize; I don't do a lot of Python and Googling hasn't worked.

I'm just trying to fetch SMS messages given a date filter with:

result = self.client.dids.get.sms(did=did, from=start, to=end)

but from is a reserved word, so this is not allowed. The code suggests that 'from' is the required parameter name, so I'm not sure how this is supposed to work.

I've tried quoting it, but then it is treated as an assignment expression and fails for a different reason. Apparently the common convention is to use a _ suffix, but that the function isn't defined that way (I also tried that and it still didn't work). Searching for things like "reserved word as keyword argument" hasn't gotten me anywhere. Perhaps there is a different way to pass keyword arguments to the method?

Apologies, again for my very basic understanding of the language.

JohnMertz commented 4 months ago

Modifying the library to use the suffix works:

626,627c626,627
<         if "from" in kwargs:
<             if not isinstance(kwargs["from"], str):
---
>         if "from_" in kwargs:
>             if not isinstance(kwargs["from_"], str):
629,630c629,630
<             validate_date(kwargs["from"])
<             parameters["from"] = kwargs.pop("from")
---
>             validate_date(kwargs["from_"])
>             parameters["from"] = kwargs.pop("from_")

However, I don't know if this is a solution that you would support or if there is a better way to assign the parameter without colliding with the reserved word, so I won't open a PR unless you ask me to.

Other API calls that also use from as a parameter include: getFaxMessages, getInvoice and getMMS.

Other API calls (eg. getVoicemailMessages) seem to use date_from and date_to instead of from and to, so it may be more intuitive to use those globally and just translate them to from and to at the assignment to parameters shown above.

JohnMertz commented 4 months ago

Proposed RP to change from -> date_from and to -> date_to in case this solution works for you: #18

JohnMertz commented 4 months ago

I fixed several other keyword collisions and in the process also implemented all missing methods, as discussed in #19