ideafast / middleware-services

Python API containing endpoints for smartphone hub applications and transfer to data portal
0 stars 0 forks source link

Assign correct default values to inventory #85

Closed jawrainey closed 3 years ago

jawrainey commented 3 years ago

Problem

As described in #80, there was once a method named name_or_none in the inventory to ensure dictionary items are correctly serialised. This method raised the following discussion:

This method feels a bit redundant imho. dict.get("name", None) already returns None if not found. Even without None, i.e.g dict.get("name") already returns None by default. For readability, I'd argue to replace the use of name_or_none() below with just the dict.get("name") method; unless I am missing something that gives an advantage here! i.e.:

model=device["model"].get("name"),
manufacturer=device["manufacturer"].get("name"),
location=device["location"]).get("name"),

_Originally posted by @davidverweij in https://github.com/ideafast/middleware-services/pull/80#discussion_r615717060_

The solution implemented was to go with a chaining option location=device.get("location", {}).get("name") if device else None, which is incorrect as location always exist in the response, but may be None if it is not set in the inventory. This breaks the current inventory use.

Solution

  1. Re-add name_or_none method
davidverweij commented 3 years ago

Good spot on the error - and not sure why I didn't catch that before the PR merge. Either way, I now see (and understand) how it checks the object first, and then performs the .get(). So the error was not not-using name_or_none, but not providing a default for get if the item was none. In the PR we accidentally removed if item else None. So, the longer version would be (can confirm this works):

model=device["model"].get("name", None) if device["model"] else None,
manufacturer=device["manufacturer"].get("name", None) if device["manufacturer"] else None,
location=device["location"].get("name", None) if device["location"] else None,

Either way, PR looks good and solves the problem! 🚀