-
-
Notifications
You must be signed in to change notification settings - Fork 188
[enhancement] improve Cardinality #5967
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
Conversation
| EXACTLY_ONE(ONE), | ||
|
|
||
| //TODO(AR) eliminate this in favour of probably ONE_OR_MORE | ||
| _MANY(MANY), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put the _ here to denote that this is a private internal value that should only be used inside eXist-db's internals. This should not be renamed. As my TODO in the code there states, if anything this should be completely removed. Removing the underscore sends a message to users that this is just any old Enum option that they can use, which is not what you or they want, as this should never be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you recall what getting rid of them entails?
I just stumbled over very recently, when fixing exist-xqts-runner to work with exist-7.0.0-SNAPSHOT
Which means that up until a certain point in the not so distant past this was changed in exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you recall what getting rid of them entails?
Yes fixing the weird code that uses it, its just a few places. You can find it via Find References in IntelliJ.
Which means that up until a certain point in the not so distant past this was changed in exist.
Actually this was done almost 6 years ago now - so definitely the distant past!
|
I would highly advise against changing anything in Cardinality, Type, Function, Basic Function, or any class that is required by a user to build an external XQuery function module in Java. As doing so will break every user's 3rd party Java Extension Module for eXist-db. Users have already experienced this pain once due to the long incubation period of 7.0.0-SNAPSHOT (~3 years) and have had to maintain two versions of their Java extensions:
If you change the APIs that are used in external function modules, then you will now force them to maintain 3 versions of their extensions. This is particularly problematic as it is being done during a long SNAPSHOT period, that makes it almost impossible for them to create a 3rd version of their extension and know whether their 2nd or 3rd version should be used against a particular 7.0.0-SNAPSHOT. If you absolutely have to do this, which I don't think you do (as it is a cosmetic change only), because of the long incubation period of 7.0.0 you should wait until 8.0.0; otherwise there will be more upset users. I don't just speak for myself here, I know multiple organisations using eXist-db for which this has been a real PITA.
There is no information in the minutes of the Community Call about this. |
BREAKING CHANGE: rename enum constants MANY to MORE and _MANY to MANY
8e266b5 to
d1a74aa
Compare
|
_MANY was introduced in 5310f05 |
|
Removing the public constant The above summarizes what was discussed on the community call. Since it was the last any other business we simply forgot to note it down. |
No it does not. No Java Library Authors should be using it, because simply there is no need for anyone to use it. AFAIK it has never been used outside of the core of eXist-db (where it should be removed also).
I am afraid that you may have completely missed the point. Please re-read what I wrote, and then if you have questions about why your statements are not correct feel free to ask. |
|
So, I understand:
Is this an accurate summary @adamretter ? |
Yes.
Yes... but also, by renaming it you are removing the signal to users that it is internal and should never be used. So you would also be encouraging users to use an API that is internal and will be removed in future (that will then also break their code again at that point). |
| return switch (cardinality) { | ||
| case EMPTY_SEQUENCE -> Cardinality.ZERO; | ||
| case EXACTLY_ONE -> Cardinality.ONE; | ||
| case _MANY -> Cardinality.MANY; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamretter so the many cardinality has to be removed in this extension as well, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@line-o Yes, if the goal is to remove _MANY then this will also need to be cleaned up too.
|
@line-o So I assume that we are agreed that you don't want to cause any more pain for eXist-db users before an 8.x.x. If so, can you close this or retarget it against a develop-8.x.x branch please? |
@adamretter in the last community meeting it was decided to leave the |
@reinhapa I am sure many people will be relieved. Thank you. |
Description:
Slightly improve developer experience when working with Java extensions.
BREAKING CHANGE: rename enum constant
MANYtoMOREand_MANYtoMANYin Cardinality.javaReference:
This PR aims to continue our discussion from todays community call 2025-12-08.
Type of tests:
none added all existing pass