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

[#890] All array types to be used as parameters #891

Merged
merged 2 commits into from
Aug 18, 2020

Conversation

andymc12
Copy link
Contributor

@andymc12 andymc12 commented Aug 3, 2020

Fixes #890 - updates to spec and javadoc enabling users to specify array types as parameters.

The javadoc contained this text: "The resulting collection is read-only." I left it alone, but I'm not sure that I like it... We could remove that text or re-word it to something like "If it is a List, Set or SortedSet it will be read-only" to avoid any confusion about "read-only arrays".
Also, I think "read-only" is not the right word here - it should probably be "immutable" or "unmodifiable". "Read-only" implies (to me) that the objects in the collection cannot be modified, but we cannot really guarantee that. IIUC, the intent is to tell the user that they can't change things like the order of the collection - or the number of entries, etc. By Java Collections terminology, that would be "unmodifiable". But even that restriction doesn't seem to make a lot of sense to me. If we lifted that restriction, users could remove query param objects from the collection and pass the modified collection to other methods (like listeners) that would then have fewer entries to filter through. That would be an argument for just removing the sentence outright. What do you all think?

Lastly, while updating the release notes page, I noticed that we still have a "since 2.2" release note page. I'll plan to remove that (from master and 3.1-SNAPSHOT) in a future PR.

@sdaschner
Copy link

LGTM

@mkarg
Copy link
Contributor

mkarg commented Aug 4, 2020

I do not think we should change the line about read-only. It does not imply read-only referenced elements, it only implies that the set of references cannot be changed. I do not see that anybody had a problem with that so far, so i wouldn't touch it. Also, arrays are not collections. For arrays, we should say that the resulting array MUST be read-only, so implementations will provide a copy to keep its internal consistency. This is essential to know for server programmers so they can rely on that mechanism and do not have to provide safety copies in all apps.

Copy link
Contributor

@mkarg mkarg left a comment

Choose a reason for hiding this comment

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

Can you please separate the "arrays" changes from the "exception mapper" changes into two distinct commits, so there is no risk of inadvertently cherry-picking each other when just one of both is wanted? Thanks. :-)

@andymc12
Copy link
Contributor Author

andymc12 commented Aug 4, 2020

I do not think we should change the line about read-only. It does not imply read-only referenced elements, it only implies that the set of references cannot be changed. I do not see that anybody had a problem with that so far, so i wouldn't touch it. Also, arrays are not collections. For arrays, we should say that the resulting array MUST be read-only, so implementations will provide a copy to keep its internal consistency. This is essential to know for server programmers so they can rely on that mechanism and do not have to provide safety copies in all apps.

That works for me.

Can you please separate the "arrays" changes from the "exception mapper" changes into two distinct commits, so there is no risk of inadvertently cherry-picking each other when just one of both is wanted? Thanks. :-)

Good idea! Done.

Please let me know if you see anything else that needs changing. Thanks!

Copy link
Contributor

@chkal chkal left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a lot!

@spericas spericas self-requested a review August 6, 2020 13:15
Copy link
Contributor

@spericas spericas left a comment

Choose a reason for hiding this comment

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

LGTM. We should create another issue to add a TCK for this.

@andymc12
Copy link
Contributor Author

andymc12 commented Aug 6, 2020

LGTM. We should create another issue to add a TCK for this.

@spericas Good idea, I opened jakartaee/platform-tck#413 to address this.

@andymc12
Copy link
Contributor Author

It's been two weeks and there are 5 +1 votes and no -1 votes. I'll merge this. Thanks all!

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

Successfully merging this pull request may close these issues.

None yet

6 participants