The serviceArea property should have a range of Place not AdministrativeArea #411

Closed
vholland opened this Issue Apr 6, 2015 · 20 comments

Comments

@vholland
Contributor

vholland commented Apr 6, 2015

The serviceArea property can be used for Places that are not AdministrativeAreas.

@vholland vholland self-assigned this Apr 6, 2015

@vholland vholland added this to the 2015 Q2 milestone Apr 6, 2015

@mfhepp

This comment has been minimized.

Show comment
Hide comment
@mfhepp

mfhepp Apr 6, 2015

Contributor

I am fine with this, but we should add a note that recommends the more granular offer -> eligibleRegion / availableAtOrFrom / delivery options patterns.

Contributor

mfhepp commented Apr 6, 2015

I am fine with this, but we should add a note that recommends the more granular offer -> eligibleRegion / availableAtOrFrom / delivery options patterns.

@jvandriel

This comment has been minimized.

Show comment
Hide comment
@jvandriel

jvandriel Apr 6, 2015

If I might ask, @mfhepp how about merging eligibleRegion and availableAtOrFrom to one property named serviceArea?

Personally I find it hard to tell when to use eligibleRegion or availableAtOrFrom, and every time I deploy either of them I always have this nagging voice in the back of my mind telling me I should have used the other property.

IMHO the difference between both properties should either be explained better - for folks like myself ;) - or we should consider merging them into serviceArea.

Or am I over-simplifying it by suggesting this?

If I might ask, @mfhepp how about merging eligibleRegion and availableAtOrFrom to one property named serviceArea?

Personally I find it hard to tell when to use eligibleRegion or availableAtOrFrom, and every time I deploy either of them I always have this nagging voice in the back of my mind telling me I should have used the other property.

IMHO the difference between both properties should either be explained better - for folks like myself ;) - or we should consider merging them into serviceArea.

Or am I over-simplifying it by suggesting this?

@mfhepp

This comment has been minimized.

Show comment
Hide comment
@mfhepp

mfhepp Apr 6, 2015

Contributor

Hmm. There is a difference, but I see the potential for making serviceArea a superproperty of both (but with a different name). eligibleRegion indicates the area to which you will be willing to deliver (for physical things) or in which the buying party resides (for services and other immaterial goods like software, music, ... availableAtOr from links to a point of sales from which you can obtain a good or service, e.g. a location from which you can rent a car, pick up a good, receive a haircut, etc.

Historically, the range of eligibleRegion was an ISO 3166 code, and that of availableAtOrFrom was a place. If we align the range, we could think about aligning the properties.

Contributor

mfhepp commented Apr 6, 2015

Hmm. There is a difference, but I see the potential for making serviceArea a superproperty of both (but with a different name). eligibleRegion indicates the area to which you will be willing to deliver (for physical things) or in which the buying party resides (for services and other immaterial goods like software, music, ... availableAtOr from links to a point of sales from which you can obtain a good or service, e.g. a location from which you can rent a car, pick up a good, receive a haircut, etc.

Historically, the range of eligibleRegion was an ISO 3166 code, and that of availableAtOrFrom was a place. If we align the range, we could think about aligning the properties.

@mfhepp

This comment has been minimized.

Show comment
Hide comment
@mfhepp

mfhepp Apr 6, 2015

Contributor

But also note that eligibleRegion is also used on gr:License and schema:DeliveryChargeSpecification. We need to double check dependencies and will likely break old data or applications. Still worth investigating.

Contributor

mfhepp commented Apr 6, 2015

But also note that eligibleRegion is also used on gr:License and schema:DeliveryChargeSpecification. We need to double check dependencies and will likely break old data or applications. Still worth investigating.

@danbri

This comment has been minimized.

Show comment
Hide comment
@danbri

danbri Sep 28, 2015

Contributor

Ok, let's attend to eligibleRegion and availableAtOrFrom separately.

The "low hanging fruit" here is to fix the range of serviceArea, which is currently inappropriately tight. I will add Place, as well as GeoShape (since some service areas aren't name-able places, so it is clunky to force an intermediate Place/geo structure here).

Contributor

danbri commented Sep 28, 2015

Ok, let's attend to eligibleRegion and availableAtOrFrom separately.

The "low hanging fruit" here is to fix the range of serviceArea, which is currently inappropriately tight. I will add Place, as well as GeoShape (since some service areas aren't name-able places, so it is clunky to force an intermediate Place/geo structure here).

@danbri

This comment has been minimized.

Show comment
Hide comment
@danbri

danbri Sep 28, 2015

Contributor

ok see http://sdo-phobos.appspot.com/serviceArea

"Values expected to be one of these types: GeoShape, Place.
Used on these types: Organization, Service"

Contributor

danbri commented Sep 28, 2015

ok see http://sdo-phobos.appspot.com/serviceArea

"Values expected to be one of these types: GeoShape, Place.
Used on these types: Organization, Service"

danbri pushed a commit that referenced this issue Sep 28, 2015

@vholland

This comment has been minimized.

Show comment
Hide comment
@vholland

vholland Sep 28, 2015

Contributor

+1

Contributor

vholland commented Sep 28, 2015

+1

@danbri

This comment has been minimized.

Show comment
Hide comment
@danbri

This comment has been minimized.

Show comment
Hide comment
@danbri

danbri Sep 28, 2015

Contributor

In fixing this I discovered another cleanup issue: we also had areaServed on ContactPoint. I have marked it supersededBy serviceArea and added ContactPoint as a type for serviceAre - see #803 for details.

Contributor

danbri commented Sep 28, 2015

In fixing this I discovered another cleanup issue: we also had areaServed on ContactPoint. I have marked it supersededBy serviceArea and added ContactPoint as a type for serviceAre - see #803 for details.

@pmika

This comment has been minimized.

Show comment
Hide comment
@pmika

pmika Sep 28, 2015

Collaborator

+1

Collaborator

pmika commented Sep 28, 2015

+1

@mfhepp

This comment has been minimized.

Show comment
Hide comment
@mfhepp

mfhepp Sep 28, 2015

Contributor

+1

Contributor

mfhepp commented Sep 28, 2015

+1

@danbri

This comment has been minimized.

Show comment
Hide comment
@danbri

danbri Sep 28, 2015

Contributor

Just posted to #803:

"@rvguha just made the sensible point that it would be better (c.f. #803) to have a property name that can't be easily mistaken for a type name. Therefore I propose we flip the supersededBy relationship around, and adopt areaServed as a (slightly) less-nouny preferred name. Unless anyone disagrees I'll update the draft site accordingly, with areaServed taking on the work now done by serviceArea. Currently we have both of these properties in parallel unrelated use, so one needs to be picked - I just think now I picked the wrong one."

Consequence here is that we continue as above, but consolidate around areaServed instead of serviceArea.

Contributor

danbri commented Sep 28, 2015

Just posted to #803:

"@rvguha just made the sensible point that it would be better (c.f. #803) to have a property name that can't be easily mistaken for a type name. Therefore I propose we flip the supersededBy relationship around, and adopt areaServed as a (slightly) less-nouny preferred name. Unless anyone disagrees I'll update the draft site accordingly, with areaServed taking on the work now done by serviceArea. Currently we have both of these properties in parallel unrelated use, so one needs to be picked - I just think now I picked the wrong one."

Consequence here is that we continue as above, but consolidate around areaServed instead of serviceArea.

@danbri

This comment has been minimized.

Show comment
Hide comment
@danbri

danbri Sep 29, 2015

Contributor

I have implemented the flip of serviceArea to areaServed (i.e. reversed the supersededBy relationship).

I have also made a pass through, declaring both eligibleRegion and availableAtOrFrom as sub-properties of the common broader areaServed property. The areaServed property list of associated types (domain and range) are amended accordingly. The definition is simply "The geographic area where a service or offered item is provided.".

@mfhepp 's account of the three properties' subtle differences I think justified their continuing distinct existence (and we could improve their descriptions to match). However it was painful that they were not even previous related to each other. I think having eligibleRegion and availableAtOrFrom as sub-properties makes sense. Note also that we have ineligibleRegion nearby, which remains untouched.

Contributor

danbri commented Sep 29, 2015

I have implemented the flip of serviceArea to areaServed (i.e. reversed the supersededBy relationship).

I have also made a pass through, declaring both eligibleRegion and availableAtOrFrom as sub-properties of the common broader areaServed property. The areaServed property list of associated types (domain and range) are amended accordingly. The definition is simply "The geographic area where a service or offered item is provided.".

@mfhepp 's account of the three properties' subtle differences I think justified their continuing distinct existence (and we could improve their descriptions to match). However it was painful that they were not even previous related to each other. I think having eligibleRegion and availableAtOrFrom as sub-properties makes sense. Note also that we have ineligibleRegion nearby, which remains untouched.

danbri added a commit that referenced this issue Sep 29, 2015

Updates for #411 - serviceArea clarified as superproperty of eligible…
…Region, availableAtOrFrom; supersedes areaServed.

This cleanup gives named relationships to these 4 very very similar-sounding properties. The associated types on
areaServed now include all those used on its sub-properties.
@chaals

This comment has been minimized.

Show comment
Hide comment
@chaals

chaals Sep 29, 2015

Contributor

Looks ok to me.

BTW, is there a way in the code to mark "associatedProperty" so that looking at eligibleRegion would lead you to ineligibleRegion?
/me hides

Contributor

chaals commented Sep 29, 2015

Looks ok to me.

BTW, is there a way in the code to mark "associatedProperty" so that looking at eligibleRegion would lead you to ineligibleRegion?
/me hides

@danbri

This comment has been minimized.

Show comment
Hide comment
@danbri

danbri Sep 29, 2015

Contributor

@chaals - I was thinking just the same. For now (like per-type deprecations) we have to write it out in the entity-escaped HTML description, which is both tedious and easy. In a way the types themselves are the real grouping construct for related properties, but whenever a property is usable on several types the assocation to related properties does become rather indirect. Let's add something.

Contributor

danbri commented Sep 29, 2015

@chaals - I was thinking just the same. For now (like per-type deprecations) we have to write it out in the entity-escaped HTML description, which is both tedious and easy. In a way the types themselves are the real grouping construct for related properties, but whenever a property is usable on several types the assocation to related properties does become rather indirect. Let's add something.

@shankarnat

This comment has been minimized.

Show comment
Hide comment

+1

@danbri

This comment has been minimized.

Show comment
Hide comment
@danbri

danbri Sep 30, 2015

Contributor

@chaals - I've cross referenced eligibleRegion and ineligibleRegion as suggested - thanks!

Contributor

danbri commented Sep 30, 2015

@chaals - I've cross referenced eligibleRegion and ineligibleRegion as suggested - thanks!

@scor

This comment has been minimized.

Show comment
Hide comment
@scor

scor Sep 30, 2015

Contributor

👍

Contributor

scor commented Sep 30, 2015

👍

@danbri

This comment has been minimized.

Show comment
Hide comment
Contributor

danbri commented Nov 6, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment