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

Would it be possible to move _create_node_from_attributes to a public API? #3

Closed
matkoniecz opened this issue Mar 6, 2023 · 7 comments

Comments

@matkoniecz
Copy link
Contributor

matkoniecz commented Mar 6, 2023

https://github.com/docentYT/osm_easy_api/blob/main/src/osm_easy_api/diff/diff_parser.py#LL79C2-L79C2

This would be useful tool for deserializing cached API responses.

As pickling cannot be safely used in this case, I am using

def deserialize_node(attributes):
    # see https://github.com/docentYT/osm_easy_api/issues/3
    # for request to move _create_node_from_attributes to
    # a public API
    node = diff_parser._create_node_from_attributes({
        'id' :            int(    attributes["id"]        ),
        'visible' :       str(attributes["visible"]).lower(), # 'true' is needed :(
        'version' :       int(    attributes["version"]   ),
        'timestamp' :     str(    attributes["timestamp"] ),
        'uid' :       int(    attributes["user_id"]       ), # inconsistent name :(
        'changeset' :  int(    attributes["changeset_id"] ), # inconsistent name :(
        'lat' :      str(    attributes.get("latitude")   ), # inconsistent name :(
        'lon' :     str(    attributes.get("longitude")   ), # inconsistent name :(
    })
    node.tags = attributes["tags"] # not passed directly
    return node

def serialize_node(node):
    return {
        'type': 'node',
        'id': node.id,
        'visible': node.visible,
        'version': node.version,
        'changeset_id': node.changeset_id,
        'timestamp': node.timestamp,
        'user_id': node.user_id,
        'tags': node.tags,
        'latitude': node.latitude,
        'longitude': node.longitude,
    }

to save responses in a database.

@matkoniecz
Copy link
Contributor Author

I can make a PR for that.

@docentYT
Copy link
Owner

docentYT commented Mar 6, 2023

serialize_node() can easily be replaced by simpler and shorter method in osm_object_primitive:

from copy import copy

def _to_dict(self):
    return_dict = copy(self.__dict__)
    return_dict.update({"type": self.__class__.__name__}) # or self.__class__.__name__.lower()
    print(return_dict)
    print(self.__dict__)

Then deserialize_node() can be replaced by the class method in the osm_object_primitive class:

@classmethod
def _from_dict(cls, dict):
    node = cls()
    dict.pop("type")
    node.__dict__ = copy(dict)
    return node

After such changes, this is how nodes will be serialised and deserialised:

from osm_easy_api import Api, Node

api = Api("https://www.openstreetmap.org")

node = api.elements.get(Node, 25733488)

dict = node._to_dict()
node_from_dict = Node._from_dict(dict)
print(node == node_from_dict) # True
print(id(node) == id(node_from_dict)) # False

What do you think about this solution?

@matkoniecz
Copy link
Contributor Author

matkoniecz commented Mar 7, 2023

I like it! Though (unless I am mistaken) _to_dict with leading underscore still denotes private function where unstable interface is allowed and expected.

(and ideally for an example user_id vs uid would be kept stable and be consistent in various places)

@matkoniecz
Copy link
Contributor Author

Looking at my code again - to enable caching of changesets it seems to be useful also there... Though at least for now it seems I will get away with caching subset of data.

@docentYT
Copy link
Owner

docentYT commented Mar 7, 2023

(and ideally for an example user_id vs uid would be kept stable and be consistent in various places)

Changing the key only in the dictionary you get is enough or is it better to consider changing it in the whole package code?

@matkoniecz
Copy link
Contributor Author

key in dictionary that is produced, that is supplied, and property name really should match

ideally also name used by API and in all package contextes would also match

@docentYT
Copy link
Owner

docentYT commented Mar 7, 2023

Done. Will be added soon as I resolve bug #4 . Renaming user_id, changeset_id, latitude and longitude should be discussed. I have added the ability to create discussions for this purpose.

@docentYT docentYT closed this as completed Mar 7, 2023
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

No branches or pull requests

2 participants