search

SNEWS2 / SNEWS_Publishing_Tools

Publishing Tool for SNEWS
BSD 3-Clause "New" or "Revised" License
2 stars 2 forks source link

Understanding signature #29

Closed Storreslara closed 2 years ago

Storreslara commented 2 years ago

Is this just looking for the argument, shouldn't we also check that it's the right type ?

https://github.com/SNEWS2/SNEWS_Publishing_Tools/blob/40301467bfd1df011e470f201361f5d3acbd0a79/SNEWS_PT/snews_pt_utils.py#L304

Storreslara commented 2 years ago

Is the overhead worth it ? considering we just check the type of the arg and appending when the condition is met ? Seems like a lot for a minor task. I do see it's benefit now that we implemented the JSON parser.

KaraMelih commented 2 years ago

Right, the way it is now dictionary.get('key', <default>) doesn't check the type, we can wrap it and say 

if type(data.get('neutrino_time', False))==str:

if there is no 'neutrino_time', default is False, and False is not a string so it should work.
I noticed that when I passed a wrong 'neutrino_time' data i.e. something that cannot be converted to date-time with the predefined format, it crashes.

I'd say for date-time objects we can even add an additional 'is format "DD/MM/YY H:M:S:f"' and if not, pad it with zeros so that all messages are comparable with each other.

For the rest of the keys, I'd say we can add a type-check if it would otherwise lead to a problem e.g. we say 'p_values' can be None type, but if it is passed like that to significance tier, something crashes on SNAP

KaraMelih commented 2 years ago

fixed.