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

Cardinality question #137

Closed
pjanck opened this issue Mar 19, 2023 · 7 comments
Closed

Cardinality question #137

pjanck opened this issue Mar 19, 2023 · 7 comments

Comments

@pjanck
Copy link
Contributor

pjanck commented Mar 19, 2023

  1. reading here:

grafik

  1. and looking at the example here:

grafik

-> I wonder: what does the combination minOccurs="1" with maxOccurs="1" mean?

@aothms
Copy link

aothms commented Mar 20, 2023

Good point.

We specifically discussed that wouldn't want to support cases like "For wall I need exactly two properties" even if the schema would allow that (hence the table).

This is first and foremost in the required category.

There's several questions here I guess:

  • Should the docs be updated?
    • State maxOccurs can be "1 or unbounded" for the required case (or leave it empty)? To disambiguate requiring exactly one or at least one?
    • Should the statement "they do not have any meaning" be refined? What does it mean: Is it allowed? Implementation dependent?
  • Is it ever meaningful to require an upperbound other than zero (because the docs seem to say "no" and I can live with that answer). In that case maybe it's better to require maxOccurs to be set to unbounded as the table suggests.

My take on this is: change the sentence to "other permutations are currently not allowed" and update the sample to unbounded.

@NickNisbet
Copy link

NickNisbet commented Mar 20, 2023 via email

CBenghi added a commit to CBenghi/IDS that referenced this issue Apr 3, 2023
Clarified the nature of the constraints on cardinality as per buildingSMART#137
@CBenghi
Copy link
Contributor

CBenghi commented Apr 3, 2023

I support @aothms view on this.
See PR #147 if agreeable.

berlotti pushed a commit that referenced this issue Apr 3, 2023
Clarified the nature of the constraints on cardinality as per #137
@CBenghi
Copy link
Contributor

CBenghi commented Apr 3, 2023

#147 has been merged. This issue can be closed.

@berlotti berlotti closed this as completed Apr 3, 2023
@pjanck
Copy link
Contributor Author

pjanck commented Apr 3, 2023

The examples weren't updated, thus making them invalid (compare 17f3b66 with point 2 above). I request to reopen the issue / should I open a new one?

@berlotti
Copy link
Member

berlotti commented Apr 3, 2023

We prefer you fix the examples and create a pull request

@CBenghi
Copy link
Contributor

CBenghi commented Apr 4, 2023

@berlotti and @pjanck,
As part of the auditing tool I'm writing, I'll take care of creating a new PR soon with the fixes for the Development files.
I'll need to liaise with @Moult for the testcases because they are programmatically generated outside of this repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants