Create a JSON schema for life cycle data#335
Conversation
e36036f to
9265a94
Compare
christoph-maurer
left a comment
There was a problem hiding this comment.
@pascalkeppler Thank you for the very good first draft of the schema! Please add commits to this branch to address the comments and let me know whenever you have questions!
|
@aklefenz @pascalkeppler Thanks for the commits! Please let me know when you have resolved all comments of the first review. Then I start the second review. |
f2af931 to
5947640
Compare
christoph-maurer
left a comment
There was a problem hiding this comment.
@pascalkeppler @aklefenz Thank you for the substantial improvement! I have fixed what I was able to fix in a day.
Please resolve the 16 comments of this second review. 11 comments are new. 5 comments are from the first review. I have marked only the old ones with @pascalkeppler @aklefenz .
|
Hi @christoph-maurer thanks for the reviews. I implemented all the feedback of your reviews, wherever possible, and resolved all comments. |
…est data and implement changes from review comments
f1cc170 to
d18029a
Compare
|
@simon-wacker I have reviewed the pull request. Unfortunately, it is already in use. I have collected my recommendations for future improvements in #358 . Do you want to review the pull request? |
simonwacker
left a comment
There was a problem hiding this comment.
I only had a quick glance and no deep understanding of the schema or the subject domain.
| "functionalUnit": { | ||
| "type": "string", | ||
| "enum": [ | ||
| "Volume", |
There was a problem hiding this comment.
For consistency with other enums in the repository use lower camel-case.
There was a problem hiding this comment.
Unfortunately it is already in use and we cannot change it, at least now. Discussed with Christoph to add it to future improvements
| "type": "number", | ||
| "description": "Terrestrial ecotoxicity potential (TETP) (kg 1,4-Dichlorobenzene)" | ||
| }, | ||
| "pere": { |
There was a problem hiding this comment.
Such abbreviations or acronyms make it hard for me to read (and understand).
There was a problem hiding this comment.
Unfortunately it is already in use and we cannot change it, at least now. Discussed with Christoph to add it to future improvements
| "type": "boolean", | ||
| "description": "The product is used as a base coat" | ||
| }, | ||
| "VocContentPerLiter": { |
There was a problem hiding this comment.
Should be lower camel-case. Also above and below.
There was a problem hiding this comment.
Unfortunately it is already in use and we cannot change it, at least now. Discussed with Christoph to add it to future improvements
| "enum": ["n.v.", "0", "1", "2", "3", "3.1", "3.2", "4", "5"] | ||
| }, | ||
| "OnlyConstructiveWoodProtection": { | ||
| "type": "boolean", |
There was a problem hiding this comment.
I'm wondering if those flags could be modeled more elegantly and if they are any interrelations. A more elegant way may be an enumeration for mutually exclusive elements of a similar kind.
There was a problem hiding this comment.
Thanks for the feedback. Unfortunately it is already in use and we cannot change it, at least now. Discussed with Christoph to add it to future improvements
| "description": "A full declaration of product ingredients is publicly available. ", | ||
| "enum": [ | ||
| "n.v.", | ||
| "available with all CAS numbers", |
There was a problem hiding this comment.
Using spaces and . makes it difficult to parse those enum members in languages like C# or Java because the enumerations there do not allow these characters. Rather use an expressive lower camel-case string with a-Z0-9 characters and add the description of those in a more readable form in the description of the enum. This also applies to other enums in this file.
There was a problem hiding this comment.
Thanks for the valuable feedback. Unfortunately it is already in use and we cannot change it, at least now. Discussed with Christoph to add it to future improvements
| "type": "string", | ||
| "description": "All substances in the product have been classified according to the Greenscreen Benchmark approach for safer chemicals", | ||
| "enum": [ | ||
| "n.v.", |
There was a problem hiding this comment.
I am wondering what n.v. stands for? no value? Anyway, I would avoid such abbreviations.
There was a problem hiding this comment.
Unfortunately it is already in use and we cannot change it, at least now. Discussed with Christoph to add it to future improvements
|
Thank you @simon-wacker ! @aklefenz @pascalkeppler Can you please resolve the conversations of @simon-wacker's review? |
|
"Already in use" I see. Too bad. Please don't reply with the same message to all conversations. I'm a human and don't want to read it so often. And I'm a bit edgy right now (also very human). |
@simon-wacker It's my fault. I should have explained the background before asking you for a review. I have asked @aklefenz to separate the recommendations for improvements in three categories.
We can talk about it in our next meeting. Sorry for the bad organization! |
|
@christoph-maurer I implemented the feedback wherever possible and pushed my commit. I left the comments open where there is sth to add to future improvements. |
|
Thank you, @aklefenz ! Which part is based on keys of Madaster? |
|
@christoph-maurer everything from |
The new domain
lifeCycleshall cover three topics:lifeCycleAssessment,circularityandcompliance. The structure oflifeCycleAssessmentandcircularityis based on a structure of Madaster. The structure ofcompliancewas developed by EPEA part of Drees&Sommer. The JSON SchemalifeCycleData.jsonwas developed by EPEA and Fraunhofer ISE. It shall include all data which is typically relevant to use life cycle data.As the data structure is already being used, the JSON Schema is not ideal. #358 collects the topics which are strongly recommended to be improved as soon as it is possible.