-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Model Update]: return request_v2.0.0 #483
Conversation
Validation Report for io.catenax.return_request/2.0.0/ReturnRequest.ttlInput model is valid |
:quantity a samm:Property ; | ||
samm:preferredName "quantity"@en ; | ||
samm:description "The desired quantity of the on the marketplace offered product in SKU."@en ; | ||
samm:characteristic :QuantityCharacteristic ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for quantity, please check if you can make use of existing shared quantity model , then you don't need to define your own Characteristic. Example here
Validation Report for io.catenax.return_request/2.0.0/ReturnRequest.ttlInput model is valid |
:startDate a samm:Property ; | ||
samm:preferredName "start date"@en ; | ||
samm:description "The first day of the period"@en ; | ||
samm:characteristic samm-c:Timestamp ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the value for characteristic should be DateTimestamp
here instead of samm-c:Timestamp
, else the example value is not correct.
:endDate a samm:Property ; | ||
samm:preferredName "end date"@en ; | ||
samm:description "The last day of the period."@en ; | ||
samm:characteristic samm-c:Timestamp ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here also, value should be DateTimestamp
|
||
:isRecall a samm:Property ; | ||
samm:preferredName "is recall"@en ; | ||
samm:description "Depicts if the manufacturer issued a recall (e.g. for security reasons). In concjunction with property needsReturn it can be expressed if the item has to be returned or can directly be destroyed."@en ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo concjunction
samm:exampleValue "PODBS39HZ7382HDG28" . | ||
|
||
:Mileage a samm-c:Measurement ; | ||
samm:preferredName "mileage"@en ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add description
here
:mileage a samm:Property ; | ||
samm:preferredName "mileage"@en ; | ||
samm:description "Current mileage of the component"@en ; | ||
samm:characteristic :Mileage ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Property and the referenced Characteristic should not have the same name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please check my comments
Validation Report for io.catenax.return_request/2.0.0/ReturnRequest.ttlInput model is valid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Validation Report for io.catenax.return_request/2.0.0/ReturnRequest.ttlInput model is valid |
@@ -0,0 +1,245 @@ | |||
####################################################################### | |||
# Copyright (c) 2023 Bayerische Motoren Werke Aktiengesellschaft |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2023-2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of them will be changed to 2023-2024.
# SPDX-License-Identifier: CC-BY-4.0 | ||
####################################################################### | ||
|
||
@prefix samm: <urn:samm:org.eclipse.esmf.samm:meta-model:2.0.0#> . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2.1.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should all the version the eclipse-related prefix be changed to 2.1.0 or just necessary for this meta-model?
As far as I understand, for making this change I would have to also upgrade my Aspect-Model-Editor to a new version, otherwise I cannot open the model after the changeis done. Is it correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct, all four have to reference 2.1.0 (which will automatically happen if you open and save it in the new version of the editor)
All updated/new models are 2.1.0
samm:description "The desired quantity of the on the marketplace offered product in SKU."@en ; | ||
samm:characteristic ext-quantity:ItemQuantityCharacteristic . | ||
|
||
:unitOfMeasure a samm:Property ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is redundant, as quantity (the shared aspect) also refers to a value and unit. Therefore maybe adapt the description of the quantity property to corrctly describe what is following in the structure below.
samm:characteristic samm-c:Text ; | ||
samm:exampleValue "8O8RKJZ7ER" . | ||
|
||
:serialNumber a samm:Property ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use of serialPart (localIdentifiers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a simple change of naming sufficient? (serialNumber->localIdentifiers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the use of the aspect serial part. Best to open the serial part model seperatly to see the redundant fields which can be linked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the aspect serial part is used, then more shared aspects are linked in the model (shared.part_site_information_as_built, business_partner_number, etc.). This means that the prefix in these shared aspect involved also needs to be updated to eclipse 2.1.0 version. I will submit multiple PRs to apply for these changes. Is it ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
samm:characteristic :DateTimestamp ; | ||
samm:exampleValue "2030-09-30"^^xsd:date . | ||
|
||
:manufacturingPartID a samm:Property ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use of serialPart (e.g manufacturerPartId)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a simple change of naming sufficient? (manufacturingPartID->manufacturerPartId)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the use of the aspect serial part
generally see requirements from the industry core @jonbckr |
# Copyright (c) 2023 ZF Friedrichshafen AG | ||
# Copyright (c) 2023 Circular Economy Solutions GmbH (C-ECO) | ||
# Copyright (c) 2023 T-Systems International GmbH | ||
# Copyright (c) 2023 Fraunhofer-Gesellschaft zur Förderung der angewandten Forschung e.V. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Contributers to Eclipse Foundation missing
@yuchengluo1 request you to resolve the comments given by Carolin. |
Validation Report for io.catenax.return_request/2.0.0/ReturnRequest.ttlInput model is valid |
All the requested adjustments are now fullfilled. |
Please look at the comments of the industry core here: |
@catroest I cannot open the miro webpage since it requires a password. |
The PW is networkofnetwork :) |
@catroest 1 question regarding the property "productGroup": Is it ok to use this name and link it with ClassificationCharacteristic from shared aspect part_classification? Or I should partClassification property directly? |
Both would be possible. If you link to the Characteristic you could describe you property yourself. Otherwise you take over the description of the partClassification property. As your descriptions are very similiar, I would suggest the "cleaner" version and link to the property, then you do not have to describe the property again. |
@yuchengluo1 let me know when the model is ready for MS2 check again. |
Validation Report for io.catenax.return_request/2.0.0/ReturnRequest.ttlInput model is valid |
You may check now, thanks. |
@prefix : <urn:samm:io.catenax.return_request:2.0.0#> . | ||
@prefix ext-characteristic: <urn:samm:io.catenax.shared.address_characteristic:3.0.0#> . | ||
@prefix ext-classification: <urn:samm:io.catenax.shared.part_classification:1.0.0#> . | ||
@prefix ext-part: <urn:samm:io.catenax.serial_part:2.0.0#> . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please update the version to 3.0.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yuchengluo1 please change version of serial_part to 3.0.0. you will first need to update your PR with main branch.
@prefix ext-characteristic: <urn:samm:io.catenax.shared.address_characteristic:3.0.0#> . | ||
@prefix ext-classification: <urn:samm:io.catenax.shared.part_classification:1.0.0#> . | ||
@prefix ext-part: <urn:samm:io.catenax.serial_part:2.0.0#> . | ||
@prefix ext-quantity: <urn:samm:io.catenax.shared.quantity:1.0.0#> . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quantity 2.0.0
Validation Report for io.catenax.return_request/2.0.0/ReturnRequest.ttlInput model is valid |
@prefix rdf: <http://www.w3.org/1999/02/22-rdf-syntax-ns#> . | ||
@prefix rdfs: <http://www.w3.org/2000/01/rdf-schema#> . | ||
@prefix xsd: <http://www.w3.org/2001/XMLSchema#> . | ||
@prefix : <urn:samm:io.catenax.return_request:3.0.0#> . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you have changed return_request version to 3.0.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, wrong touch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please make commented changes before merge.
Validation Report for io.catenax.return_request/2.0.0/ReturnRequest.ttlInput model is valid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
As mentioned in issue: #313.
-->
Closes #313
MS2 Criteria
(to be filled out by PR reviewer)
DismantlerId
andDismantlerName
use an EntityDismantler
with the propertiesname
andid
or use a URN likeio.catenax.dismantler:0.0.1
)preferredName
anddescription
are not the samepreferredName
should be human readable and follow normal orthography (e.g., no camel case but normal word separation)MS3 Criteria
(to be filled out by semantic modeling team before merge to main-branch)