iss #193 fix 'optional' and 'not null' conflict

[stephenholleran]

Pull request to review issue #193.

[stephenholleran]

Hi @dancasey-ie

I have made those changes. Can you please review?

Thanks,

Stephen

[dancasey-ie]

@stephenholleran I am not sure if they were noted previously but null should also be added to the enum list for sensor_type_id and orientation_reference. No changes to the SQL are required for these.

[stephenholleran]

@stephenholleran I am not sure if they were noted previously but null should also be added to the enum list for sensor_type_id and orientation_reference. No changes to the SQL are required for these.

There were actually a few more in 'device_vertical_orientation' and 'mounting_type_id'.

[stephenholleran]

Hi @dancasey-ie,

I have made all the updates based on your comments both here and in #193.

Could you please review and give your approval. (Just a note here to say you are ok with it as I don't think you have reviewer privilege's.)
I think we should have split the 'date_from' into a different issues/PR as it is a slightly separate issue. I hope the PR is not too big.

Thanks,

[dancasey-ie]

@stephenholleran all looks correct

[stephenholleran]

Thanks @dancasey-ie.

@abohara are you able to take a quick look before I merge?

[abohara]

@stephenholleran Some feedback below:

Why is date_from optional in lidar config ?

If the units are optional these changes make sense. I think it is prudent to wait just a little bit before hitting merge so that we can conclude the discussions in issue [https://github.com//issues/200]. We can try wrap up the conversation this Thursday perhaps ?

[stephenholleran]

Hi @abohara ,

Thanks for reviewing.

Why is date_from optional in lidar config ?

Can't really remember. Thinking about it now they probably should. However, to make it required would mean a breaking change so I think we can come back to this for a v2 release. I have create a new issues for this #208.

If the units are optional these changes make sense. I think it is prudent to wait just a little bit before hitting merge so that we can conclude the discussions in issue [https://github.com/https://github.com//issues/200]. We can try wrap up the conversation this Thursday perhaps ?

I don't think this PR needs to be held up by that discussion. If we decide to make the units required we can make those changes for that issue. Again, that would be a breaking change and so should be in a v2 release.

[abohara]

Thanks @stephenholleran . The changes look good and thank you for sorting out how required vs optional are represented.