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

Adding units to the slope and offset in the calibration table #262

Closed kersting closed 3 months ago

kersting commented 4 months ago

Issue https://github.com/IEA-Task-43/digital_wra_data_standard/issues/248

stephenholleran commented 4 months ago

Hi @kersting,

Thanks for having a go at this. A few comments:

  1. I commented on the changelog entries are just under the last release and not under the "Unreleased" heading.

  2. Similar to Amit's comment about the type in the SQL, the type in the SCHEMA should be 'string' and not 'number. I ran your sample through a validator and it highlights these errors. image

  3. I think the units should be an enum and not be a free field for anyone to type anything? If we agree then there is a bit of work in moving the 'measurement_units' out of the 'logger_measurement_config' area into it's own definition. I can help with that. I don't think this would make a breaking change. @abohara do you think it would make a breaking change?

  4. In the SQL statements the measurement_units are already in a table on their own so if we agree with above, then we just need to add this as a foreign key to the 'Calibration' table.

Thanks,

kersting commented 4 months ago

@abohara and @stephenholleran sorry for the mistakes. The habit of copying and pasting without thinking... I fixed everything in my local but I didn't make a new pull request based on @stephenholleran comments about making into an enum. From here we have a couple of options. I can just update the pull request so @stephenholleran can pick it from here or I can also do the enum part. Please let me know if you have a preference.

stephenholleran commented 3 months ago

Hi @kersting,

Just push your changes to GitHub and they'll appear here. I can then pull this updated branch and move the measurement_units into a definition.

Thanks,

stephenholleran commented 3 months ago

Hi @kersting,

Did you merge by mistake? There are still a few of my issues outstanding and we have to move measurement_units into a definition. I meant to just push the changes you had on your local computer into this branch and then I could see them here. Not really to merge the PR.

There seems to be a 'revert' button just above here in the line item where it says it has merged. Maybe you could try that to revert this PR?

Cheers, Stephen

kersting commented 3 months ago

@stephenholleran sorry I misunderstood what you wrote. Let me revert it.

kersting commented 3 months ago

@stephenholleran I created one to revert and created a brand new one with the fixes.