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

Remove TimeZone & etc. Fixes #650 #815

Merged
merged 6 commits into from
Apr 18, 2023

Conversation

Jamie-SA
Copy link
Contributor

@Jamie-SA Jamie-SA commented Apr 13, 2023

Fixes #650

Removes

  • gist:TimeZone
  • gist:TimeZoneStandard
  • gist:hasOffsetToUniversal
  • gist:usesTimeZoneStandard

@Jamie-SA Jamie-SA requested review from rjyounes and dylan-sa April 13, 2023 16:51
@Jamie-SA Jamie-SA added the impact: major Non-backward compatible (changes inferences; e.g., adding a restriction, domain, range) label Apr 14, 2023
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.

Just a couple minor things on the release notes; otherwise looking good to me.

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

Addresses issue #650.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could add the link here. #650

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to see some descriptive text here, as we typically do with our release notes. For example:

  • Renamed gist:Task, gist:ScheduledTask and gist:Project to gist:TaskExecution, gist:ScheduledTaskExecution and gist:ProjectExecution, respectively. Issue #590.

This includes the link and backticks that Dylan has suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rjyounes I'm not sure what more information you would like to see. Your example shows saying those things were renamed. Below I've already said what things were removed. I think what I have is similar to your example.

Addresses issue #650.

Removes
- gist:TimeZone
Copy link
Contributor

Choose a reason for hiding this comment

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

Backticks for the IRIs.

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

Addresses issue #650.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to see some descriptive text here, as we typically do with our release notes. For example:

  • Renamed gist:Task, gist:ScheduledTask and gist:Project to gist:TaskExecution, gist:ScheduledTaskExecution and gist:ProjectExecution, respectively. Issue #590.

This includes the link and backticks that Dylan has suggested.

gistCore.ttl Outdated
@@ -2854,7 +2820,7 @@ The subproperties allow the ontologist to do three things:

All datetimes are of the same format: '2021-06-01T08:03:27.12324-6:00'^^xsd:dateTime. This is compatible with and a subset of ISO 8601.

Time zone offset, such as -6:00 (of which there are a few dozen) is recognized in the date itself, as shown. The actual time zone standard (of which there are 131) may optionally be attached to the event or other object itself; see property gist:usesTimeZoneStandard.
Time zone offset, such as -6:00 (of which there are a few dozen) is recognized in the date itself, as shown. The actual time zone standard (of which there are 131) may optionally be attached to the event or other object itself.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to get rid of the second sentence about time zone standard, since we couldn't agree yesterday about what a time zone standard is. Since we deleted the property the comment isn't very relevant anyway - I.e., we are not providing a way to attach the standard to the event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the second sentence is useful information, even if we don't provide a mechanism to do this. I do think the first sentence could be improved and I'll think about changes to the second sentence, but I don't see the need to remove the second sentence.

@Jamie-SA Jamie-SA requested review from rjyounes and dylan-sa April 17, 2023 15:14
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.

One requested change, otherwise looks good.

gistCore.ttl Outdated
@@ -2854,7 +2820,7 @@ The subproperties allow the ontologist to do three things:

All datetimes are of the same format: '2021-06-01T08:03:27.12324-6:00'^^xsd:dateTime. This is compatible with and a subset of ISO 8601.

Time zone offset, such as -6:00 (of which there are a few dozen) is recognized in the date itself, as shown. The actual time zone standard (of which there are 131) may optionally be attached to the event or other object itself; see property gist:usesTimeZoneStandard.
A time zone offset, such as -6:00, can be included as part of the datetime itself, as shown. The offset is not the same as a time zone. There are roughly a few dozen time zone offsets and around 130 named time zones. If needed, a time zone may be recorded by attaching it to the event or other object.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If needed, a time zone may be recorded by attaching it to the event or other object.

Since we are removing the classes, this should also be removed.

@Jamie-SA Jamie-SA requested a review from rjyounes April 18, 2023 14:52
@rjyounes
Copy link
Collaborator

@dylan-sa Can you review Jamie's changes in response to your review comments, and approve it or submit a new request for changes? This is close to being ready to merge to develop.

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.

Looks good.

@rjyounes rjyounes merged commit 2b3b55c into develop Apr 18, 2023
@rjyounes rjyounes deleted the issue-650-remove-timezone-and-related branch April 18, 2023 17:05
@rjyounes rjyounes mentioned this pull request May 2, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact: major Non-backward compatible (changes inferences; e.g., adding a restriction, domain, range)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues relating to TimeZone and TimeZoneStandard
3 participants