Skip to content

Conversation

bernardnormier
Copy link
Member

This PR updates the JavaBean-style mapping field to be private, just like for optional fields.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR makes JavaBean-style mapping fields private to maintain consistency with optional fields. The change ensures that both optional fields and JavaBean fields (with getter/setter methods) follow the same encapsulation pattern by being declared as private instead of public.

  • Updates field visibility logic to include JavaBean-style fields alongside optional fields
  • Maintains consistent encapsulation patterns across different field types

}

if (isOptional)
if (getSet || isOptional)
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

[nitpick] The condition combines two different concepts (getSet and isOptional) without clear documentation. Consider adding a comment explaining why both JavaBean fields and optional fields should be private, or extract this logic into a well-named boolean variable like 'shouldBePrivate' for better readability.

Suggested change
if (getSet || isOptional)
// Fields should be private if they are JavaBean properties (getSet) or optional fields (isOptional),
// to ensure encapsulation for getter/setter methods and to manage presence tracking for optionals.
const bool shouldBePrivate = getSet || isOptional;
if (shouldBePrivate)

Copilot uses AI. Check for mistakes.

Copy link
Member

@InsertCreativityHere InsertCreativityHere left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@bernardnormier bernardnormier merged commit 0e48fdc into zeroc-ice:main Aug 6, 2025
27 checks passed
pepone pushed a commit to pepone/ice that referenced this pull request Sep 16, 2025
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

Successfully merging this pull request may close these issues.

2 participants