Skip to content
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

Deprecated properties related to occupying a location. #941

Merged
merged 8 commits into from
Aug 16, 2023

Conversation

uscholdm
Copy link
Contributor

@uscholdm uscholdm commented Jul 27, 2023

Fixes #809.

See #947 for what to do on the next major release.

Minor Updates

  • Deprecated properties related to occupying a location. Issue #809. Changes:
    • Deprecated properties: gist:occupiesGeographically and gist:occupiesGeographicallyPermanently.
    • Updated annotations for gist:hasPhysicalLocation regarding location temporality.
    • Added editorialNote on gist:Landmark to update the restriction on the next major release.

@uscholdm uscholdm requested review from rjyounes and dylan-sa July 27, 2023 18:26
Copy link
Collaborator

@rjyounes rjyounes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about this, I forgot the current protocol for deprecation. I'm going to update the issue with the new implementation decision.

@@ -0,0 +1,6 @@
### Major Updates

- Removed properties related to occupying a location. Issue [#809](https://github.com/semanticarts/gist/issues/809). Changes:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Removed properties related to occupying a location. Issue [#809](https://github.com/semanticarts/gist/issues/809). Changes:
- Deprecated properties related to occupying a location. Issue [#809](https://github.com/semanticarts/gist/issues/809). Changes:


- Removed properties related to occupying a location. Issue [#809](https://github.com/semanticarts/gist/issues/809). Changes:
- Removed properties: `gist:occupiesGeographically` and `gist:occupiesGeographicallyPermanently`.
- Updated restriction on `gist:Landmark` due to property being removed.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Updated restriction on `gist:Landmark` due to property being removed.

uscholdm and others added 3 commits July 28, 2023 15:51
Co-authored-by: Rebecca Younes <rebecca.younes@semanticarts.com>
@uscholdm uscholdm requested a review from rjyounes July 28, 2023 20:36
Copy link
Contributor

@dylan-sa dylan-sa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just had one clarification question on the new deprecation policy. Otherwise looking good to me!

gistCore.ttl Outdated
@@ -1287,6 +1287,7 @@ gist:Landmark
) ;
] ;
skos:definition "Something permanently attached to the Earth."^^xsd:string ;
skos:editorialNote "On the next major release gist:occupiesGeographicallyPermanently will be gone. The restriction above will use gist:hasPhysicalLocation. Use subClassOf instead of equivalentClass."^^xsd:string ;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm refreshing my memory on the new deprecation policy. One part of the policy is:

If the term slated for removal is referenced by other term definitions, as in a domain axiom or property restriction, such that a change to that reference constitutes a major change, the term cannot be deprecated in a minor release. The change or removal must wait until the next major release, when the references to it can be appropriately modified or removed.

In this case, the term slated for removal (occupiesGeographicallyPermanently) is used in a property restriction. If we were to use hasPhysicalLocation in the restriction instead, would that rise to the level of a major change, meaning that occupiesGeographicallyPermanently should not be deprecated in a minor release? Am I interpreting this part of the policy correctly? @rjyounes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would that rise to the level of a major change, meaning that occupiesGeographicallyPermanently should not be deprecated in a minor release

That is how I interpreted it, and that's why I left it as is.

Copy link
Collaborator

@rjyounes rjyounes Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I'm not sure I correctly captured our intent in the documentation. I think we might have said that we can deprecate the term, but not change the restrictions or other references. @marksem Do you recall or have an opinion? @bpelakh You may want to weigh in on this as well. I'm in favor of the latter, in which case we can deprecate the terms and not change anything else.

@rjyounes
Copy link
Collaborator

rjyounes commented Aug 7, 2023

I'm inclined to hold hard to the line that any change in inferencing outcomes constitutes a major change; otherwise it becomes too subjective. In software development, one would not say "Yeah, changing this method name breaks backward compatibility, but no one uses the method anyway, so let's call it a minor change." I recommend closing this PR and tagging the issue as "deferred to major release." Let's bring this up at a meeting because it's a significant issue that should be resolved as a group.

@rjyounes
Copy link
Collaborator

MarkW: Policy should be to deprecate (with OWL annotation) but keep any references as is.

Given this policy, the PR should not represent a major change. Michael will check.

Rebecca: check documentation.

@uscholdm uscholdm changed the title Removed properties related to occupying a location. Deprecated properties related to occupying a location. Aug 10, 2023
@uscholdm uscholdm requested review from dylan-sa and marksem August 10, 2023 17:13
@rjyounes
Copy link
Collaborator

@uscholdm Please remember to add "Fixes #nnn" to the description of the PR so that when the PR is merged, the issue is automatically closed. This also helps readers easily find the issue.

uscholdm and others added 2 commits August 16, 2023 15:42
Co-authored-by: Rebecca Younes <rebecca.younes@semanticarts.com>
Co-authored-by: Rebecca Younes <rebecca.younes@semanticarts.com>
@uscholdm
Copy link
Contributor Author

@uscholdm Please remember to add "Fixes #nnn" to the description of the PR so that when the PR is merged, the issue is automatically closed. This also helps readers easily find the issue.

Done

@uscholdm uscholdm requested a review from rjyounes August 16, 2023 19:55
@rjyounes rjyounes merged commit d34357b into develop Aug 16, 2023
@rjyounes rjyounes deleted the Issue-809-occupiesGeographically branch August 16, 2023 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Remove occupiesGeographically and occupiesGeographicallyPermanently?
3 participants