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

proposed rewrite of embeddables/associations #392

Merged
merged 8 commits into from
Dec 14, 2023
Merged

proposed rewrite of embeddables/associations #392

merged 8 commits into from
Dec 14, 2023

Conversation

gavinking
Copy link
Contributor

This is a work in progress. Leaving it here so that people can post comments.

@gavinking gavinking marked this pull request as draft December 7, 2023 22:43
@gavinking
Copy link
Contributor Author

By the way, I'm inclined to think that we really don't need to say very much at all here. Any support for associations in particular is going to be extremely datastore-specific.

njr-11
njr-11 reviewed Dec 7, 2023
View reviewed changes
Copy link
Contributor

@njr-11 njr-11 left a comment

Choose a reason for hiding this comment

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

This looks great. I added a few review comments that are mostly grammar updates a few minor potential improvements to consider.

spec/src/main/asciidoc/repository.asciidoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/repository.asciidoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/repository.asciidoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/repository.asciidoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/repository.asciidoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/repository.asciidoc Show resolved Hide resolved
@gavinking
Copy link
Contributor Author

This looks great. I added a few review comments that are mostly grammar updates a few minor potential improvements to consider.

Thanks, I accepted all your proposed changes except for the last one, which I would like to discuss further.

@otaviojava
Copy link
Contributor

By the way, I'm inclined to think that we really don't need to say very much at all here. Any support for associations in particular is going to be extremely datastore-specific.

I like this approach.

@gavinking
Copy link
Contributor Author

I've added a new passage addressing #393, at least in part. WDYT?

@gavinking gavinking force-pushed the embeddables-associations branch from 3bc4dc2 to 97b7e76 Compare December 11, 2023 17:37
@gavinking gavinking marked this pull request as ready for review December 11, 2023 18:05
gavinking
gavinking commented Dec 11, 2023
View reviewed changes
spec/src/main/asciidoc/repository.asciidoc
@@ -210,10 +210,15 @@ Furthermore, the programming model must specify how the Jakarta Data provider ac

Every programming model for entity classes must support _direct field access_, that is, access to the persistent fields of an entity class without triggering any intermediating user-written code such as JavaBeans-style property accessors.
When direct field access is used, every Java field marked with the Java language `transient` modifier must be treated as transient.
A programming model might place constraints on the visibility of persistent fields.
For example, Jakarta Persistence disallows `public` persistent fields.
Every programming model must permit `private` persistent fields.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Folks please note this language that I just added.

@gavinking gavinking changed the title initial very rough draft of rewrite of embeddables/associations proposed rewrite of embeddables/associations Dec 11, 2023
@otaviojava otaviojava merged commit 36880b1 into jakartaee:main Dec 14, 2023
3 checks passed
@gavinking
Copy link
Contributor Author

Thanks, @otaviojava!

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

@njr-11 njr-11 njr-11 approved these changes

@otaviojava otaviojava otaviojava approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@gavinking @otaviojava @njr-11