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

Class for statevars and argsdef as dict #4

Open
smaiLee opened this issue Aug 31, 2017 · 4 comments
Open

Class for statevars and argsdef as dict #4

smaiLee opened this issue Aug 31, 2017 · 4 comments

Comments

@smaiLee
Copy link
Contributor

smaiLee commented Aug 31, 2017

What do you think about a class for representing statevars?
Currently they are just a dict in another dict Service.statevars

And another related wish:
Statevars are held in argsdef_in and argsdef_out as a list of tuples of arg_name and the actual statevar.
Would you mind if I change argsdef_in and _out to dictionaries of { arg_name: statevar }
This would make processing of an action result simpler (currently I have to convert the argsdef_out to a dict first).

@flyte
Copy link
Owner

flyte commented Aug 31, 2017

Yeah, that could do with a bit of attention, you're right. I wonder if there's a way to include their uPnP 'type' so that we can provide (un)marshal and validation methods too.

I'm happy with changing argsdef_(in|out) to dictionaries. There's never a case where two arguments share the same key, so go for it.

smaiLee added a commit to smaiLee/upnpclient that referenced this issue Aug 31, 2017
smaiLee added a commit to smaiLee/upnpclient that referenced this issue Aug 31, 2017
Includes validation and type marshaling (as discussed in flyte#4)
@smaiLee
Copy link
Contributor Author

smaiLee commented Aug 31, 2017

I just did the work and refactored the argsdef lists to dicts and implemented the class Statevar with methods validate_arg and marshal_value.

It's completely untested yet as I currently have no UPnP devices around me (and not even a Python runtime, indeed). I will test it this evening and make a PR.
You can preview the argsdef dict at 5199a14 and the statevar class at 5f4c251 if you like.

The marshal_value method just calls the existing marshal.py. Do you think it would be better to move the whole functionality to the method on Statevar and remove marshal.py?

Do you mind backward compatibility? Then I could add __getitem__ to simulate dict-like value access like before:

    def __getitem__(self, key):
        """
        Allow attributes to be returned as dictionary keys (for backward compatibility).
        """
        return getattr(self, key)

@flyte
Copy link
Owner

flyte commented Aug 31, 2017

Good work! I had a quick look through just now - looks good. If you've got time, I'd appreciate it if you could update the Readme to reflect the changes, as it's now wrong. You may find there are some tests which fail now too.

I'd like to keep marshal.py I think, as it keeps that stuff in its own place. If anybody wants to use it for another reason then it makes it less cumbersome than having to use our classes.

I'm not that fussed about backward compatibility at the moment. Anyone can still use getattr if the param name is in a variable anyway, so it's probably not necessary.

PS. I'm on holiday for two weeks from Saturday, so if I stop replying, please don't get offended :)

@smaiLee
Copy link
Contributor Author

smaiLee commented Aug 31, 2017

I will have a look at Readme and examples and run the tests. As mentioned, this was completely "blind coding" as I could not even run it in Python.

I completely agree to use marshal.py.

Well, it's hard as I did already got used to your fast answering. 😄 But have a great holiday!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants