-
Notifications
You must be signed in to change notification settings - Fork 881
Schema changes for #3617 on Shipping Details representation. #3682
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
Conversation
ec4272b to
ecf80af
Compare
|
The design doc and the proposed change went through the the link to the new Type is now: https://1-dot-schemadotorg1.ew.r.appspot.com/ShippingConditions |
77c8c98 to
d47e82b
Compare
|
This pull request is being nudged due to inactivity. |
d47e82b to
8894764
Compare
|
Newly deployed here now: https://1-dot-schemadotorg1.ew.r.appspot.com/ShippingService |
40236a8 to
de74694
Compare
data/ext/pending/issue-3617.ttl
Outdated
| # ║ FulfillmentType enumeration ║ | ||
| # ╚════════════════════════════════════════════════════════╝ | ||
|
|
||
| :FulfillmentType a rdfs:Class ; |
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.
change to FulfillmentTypeEnumeration for consistency with other enumeration types
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.
done.
data/ext/pending/issue-3617.ttl
Outdated
|
|
||
| :FulfillmentType a rdfs:Class ; | ||
| rdfs:label "FulfillmentType" ; | ||
| rdfs:comment "A type of product shipping fulfillment." ; |
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.
"shipping" seems superfluous, maybe just "A fulfillment method" ?
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.
done.
data/ext/pending/issue-3617.ttl
Outdated
| rdfs:label "Delivery" ; | ||
| :isPartOf <https://pending.schema.org> ; | ||
| :source <https://github.com/schemaorg/schemaorg/issues/3617> ; | ||
| rdfs:comment "Shipping service fulfilling items to a customer selected address." . |
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.
"Fulfillment to a customer's selected address." ? (same prefix for the other enum value below) That way we make it more generic
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.
done.
| :isPartOf <https://pending.schema.org> ; | ||
| :source <https://github.com/schemaorg/schemaorg/issues/3617> . | ||
|
|
||
| :validForMemberTier a rdf: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.
need to update the description for this existing property, it refers to OfferShippingDetails but should now refer also to ShippingService. Maybe edit this entire property in the other .ttl file where the property was originally defined (for issue 3563).
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 think this should be renamed deliveryTime so it is consistent with the rest of the vocabulary.
data/ext/pending/issue-3617.ttl
Outdated
| rdfs:subClassOf :StructuredValue ; | ||
| :isPartOf <https://pending.schema.org> ; | ||
| :source <https://github.com/schemaorg/schemaorg/issues/3617> ; | ||
| rdfs:comment "ShippingConditions represent a set of constraints and information about the conditions of shipping a product. Such conditions may apply to only a subset of the products being shipped, depending on aspects of the product like weight, size, price, destination, and others." . |
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 would add something that makes explicit that these conditions together specify a resulting shipping cost and/or handling and transit time. For example heavy or bulky items will be more expensive to ship that small items.
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.
added some clarification.
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.
Ideally we also need some examples (in a separate issue-3617-examples.txt file)
6b201bc to
6959ef2
Compare
data/ext/pending/issue-3617.ttl
Outdated
| :domainIncludes :OfferShippingDetails ; | ||
| :rangeIncludes :Mass . | ||
|
|
||
| :shippingService a rdf: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.
do we need this since we also add :hasShippingService below? We can have the same property used on both :OfferShipping details and :Organization
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.
done. Even though the 'has' prefix is confusing users out there...
| # ╔════════════════════════════════════════════════════════╗ | ||
| # ║ Delete DeliveryTimeSettings ║ | ||
| # ╚════════════════════════════════════════════════════════╝ | ||
| :DeliveryTimeSettings a rdfs:Class ; |
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 this also go to the attic then?
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.
Done.
| # ╔════════════════════════════════════════════════════════╗ | ||
| # ║ Adding ShippingCondition ║ | ||
| # ╚════════════════════════════════════════════════════════╝ | ||
| :ShippingConditions a rdfs:Class ; |
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 this be singular instead of plural since it is a single condition? (comment above is also singular)
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.
corrected the comment. There are actually multiple conditions in that target Type.
data/ext/pending/issue-3617.ttl
Outdated
| :rangeIncludes :QuantitativeValue ; | ||
| :isPartOf <https://pending.schema.org> ; | ||
| :source <htps://github.com/schemaorg/schemaorg/issues/3617> ; | ||
| rdfs:comment "Limits in the number of items being shipped for which these conditions apply." . |
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.
Limits the number (no "in" ?)
data/ext/pending/issue-3617.ttl
Outdated
| :rangeIncludes :Boolean ; | ||
| :source <htps://github.com/schemaorg/schemaorg/issues/3617> . | ||
|
|
||
| :deliveryTime a rdf: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.
does this represent transit time only and then not be named as such? (since handling time is modeled under ShippingService)? Or does it model both transit time and handling time (but in that case we need two separate properties IIUC)?
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.
noticed it is also defined below?
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 had not updated it to the new ServicePeriod.
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.
these examples result in some build errors against the schema
6959ef2 to
1950a94
Compare
data/ext/pending/issue-3617.ttl
Outdated
| # ╔════════════════════════════════════════════════════════╗ | ||
| # ║ Amend ShippingDeliveryTime ║ | ||
| # ╚════════════════════════════════════════════════════════╝ | ||
| :transitDays a rdf: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.
do we also need to add :handlingDays ?
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.
that is cruft thatI remove now.
data/schema.ttl
Outdated
| In the context of [[OfferShippingDetails]] and [[ShippingService]], this field is used to specify the handling time and the shipping time if the merchant does the shipping. | ||
| In the context of [[ShippingConditions]], this is generally used to specify the transit time.""" ; |
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 no longer under ShippingConditions? And do we still need :delivery time now we have :handling time and :transitTime?
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.
since we have handlingTime and transitTime now, this comment clash no longer applies. Reverted to the previous message.
This changes the representation of shipping information accordoing to issue schemaorg#3617, with some properties moved to the attic.
54a65a1 to
5ed2ba5
Compare
5ed2ba5 to
8c273a0
Compare
This changes the representation of shipping information according to issue #3617, with some properties moved to the attic.
See details in the design doc.
The new structure can be navigated at: https://1-dot-schemadotorg1.ew.r.appspot.com/ShippingConditions