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.

Sign up for GitHub

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

Jump to bottom

Domain range includes. Fixes #389 #479

Merged
merged 11 commits into from
May 28, 2021
Merged

Domain range includes. Fixes #389 #479

merged 11 commits into from
May 28, 2021

Conversation

uscholdm
Copy link
Contributor

Addressing issue #389

uscholdm added 3 commits May 19, 2021 13:24
@uscholdm
Addressing issue: #389
433b3a1 
- added two annotation properties: domainIncludes and rangeIncludes
@uscholdm
Addressing issue: #389
0e4f311 
- updated release notes
@uscholdm
Addressing issue: #389
b4ee45c 
- added specific uses of domainIncludes and rangeIncludes
@uscholdm
Addressing issue: #389
2616544 
- updated release notes
sa-bpelakh
sa-bpelakh approved these changes May 19, 2021
View reviewed changes
Copy link
Collaborator

@sa-bpelakh sa-bpelakh left a comment

Choose a reason for hiding this comment

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

Changes look fine, that's actually very useful. Small suggestion on release notes.

@uscholdm
Addressing issue: #389
c1fd197 
- updated release notes (again)
@uscholdm uscholdm mentioned this pull request May 25, 2021
Merged
@uscholdm uscholdm requested a review from sa-bpelakh May 25, 2021 18:00
@uscholdm uscholdm linked an issue May 25, 2021 that may be closed by this pull request
Add annotation properties to indicate intended usage of properties AND 'min 0' begone. #389
Closed
rjyounes
rjyounes requested changes May 25, 2021
View reviewed changes
@uscholdm
Addressing issue: #389
ba19d24 
- made a few tweaks to tidy things up a bit
- updated release notes (again)
@uscholdm uscholdm requested a review from rjyounes May 26, 2021 00:52
rjyounes
rjyounes requested changes May 26, 2021
View reviewed changes
Copy link
Collaborator

@rjyounes rjyounes left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Two minor changes. I'd like to hear @marksem's take on the patch vs minor update issue, then once that's resolved we're good to go.

Either way I should make this explicit in the documentation.

@rjyounes rjyounes closed this May 27, 2021
@rjyounes rjyounes deleted the domain-range-includes branch May 27, 2021 17:49
@rjyounes rjyounes restored the domain-range-includes branch May 28, 2021 13:38
@rjyounes
Copy link
Collaborator

Closed by mistake: meant to merge and close.

@rjyounes rjyounes reopened this May 28, 2021
@rjyounes rjyounes merged commit a3af728 into develop May 28, 2021
@rjyounes rjyounes deleted the domain-range-includes branch May 28, 2021 13:39
@rjyounes rjyounes changed the title Domain range includes Domain range includes. Fixes #389 May 28, 2021
@rjyounes rjyounes mentioned this pull request May 28, 2021
Closed
@rjyounes
Copy link
Collaborator

We concluded in the gist issue review meeting that addition of annotations is a minor change because it is an addition. Created new issue #488 to document this.

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

@rjyounes rjyounes rjyounes approved these changes

@marksem marksem Awaiting requested review from marksem

@sa-bpelakh sa-bpelakh Awaiting requested review from sa-bpelakh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add annotation properties to indicate intended usage of properties AND 'min 0' begone.
3 participants
@uscholdm @rjyounes @sa-bpelakh