search

IEA-Task-43 / digital_wra_data_standard

IEA Task 43: pre-construction energy estimate data standard repository
BSD 3-Clause "New" or "Revised" License
58 stars 16 forks source link

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

Closed stephenholleran closed 1 year ago

stephenholleran commented 1 year ago

Pull request to review issue #193.

stephenholleran commented 1 year ago

Hi @dancasey-ie

I have made those changes. Can you please review?

Thanks,

Stephen

dancasey-ie commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@stephenholleran all looks correct

stephenholleran commented 1 year ago

Thanks @dancasey-ie.

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

abohara commented 1 year ago

@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/IEA-Task-43/digital_wra_data_standard/issues/200]. We can try wrap up the conversation this Thursday perhaps ?

stephenholleran commented 1 year ago

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/IEA-Task-43/digital_wra_data_standard/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 commented 1 year ago

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