Skip to content

refactor: make objects immutable and remove redundant methods - #1002

Open
bengbengbalabalabeng wants to merge 5 commits into
apache:mainfrom
bengbengbalabalabeng:refactor-value-object-cleanup
Open

refactor: make objects immutable and remove redundant methods#1002
bengbengbalabalabeng wants to merge 5 commits into
apache:mainfrom
bengbengbalabalabeng:refactor-value-object-cleanup

Conversation

@bengbengbalabalabeng

Copy link
Copy Markdown
Contributor

Purpose of the pull request

As title.

What's changed?

  • Remove some unused Lombok annotations.
  • Refactor these classes with only read-only properties into immutable objects.

Checklist

  • I have read the Contributor Guide.
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 refactors several metadata.property value-holder classes in fesod-sheet toward immutability by removing Lombok setters and switching a subset of properties to final fields + constructor-based creation, along with some minor test adjustments.

Changes:

  • Convert multiple annotation-derived property classes to constructor-initialized immutable objects (final fields + @AllArgsConstructor).
  • Remove Lombok-generated setters (and some other Lombok annotations) from style/font-related property classes and update internal builders to assign fields directly.
  • Adjust freeze-pane strategy unit tests by removing the property-constructor test.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
fesod-sheet/src/test/java/org/apache/fesod/sheet/write/style/SheetFreezePaneStrategyTest.java Removes a unit test that previously exercised the property-based constructor
fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/property/StyleProperty.java Removes Lombok setters and updates builder methods to assign fields directly
fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/property/SheetFreezePaneProperty.java Makes freeze-pane property immutable via final fields + all-args ctor; builder now constructs directly
fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/property/RowHeightProperty.java Switches to final field + @AllArgsConstructor
fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/property/OnceAbsoluteMergeProperty.java Switches to final fields + @AllArgsConstructor
fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/property/NumberFormatProperty.java Switches to final fields + @AllArgsConstructor
fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/property/LoopMergeProperty.java Switches to final fields + @AllArgsConstructor
fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/property/FontProperty.java Removes Lombok setters and updates builder methods to assign fields directly
fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/property/DateTimeFormatProperty.java Switches to final fields + @AllArgsConstructor
fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/property/ColumnWidthProperty.java Switches to final field + @AllArgsConstructor
Suppressed comments (1)

fesod-sheet/src/main/java/org/apache/fesod/sheet/metadata/property/SheetFreezePaneProperty.java:71

  • getOrDefault only treats -1 as the sentinel value. Per the annotation semantics, any negative value (e.g. -5) should fall back to the default; otherwise SheetFreezePaneStrategy(SheetFreezePaneProperty) will throw due to its >= 0 validation when users accidentally provide other negative numbers.
    private static Integer getOrDefault(Integer value, Integer defaultValue) {
        if (value == -1) {
            return defaultValue;
        }
        return value;

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@alaahong

Copy link
Copy Markdown
Member

generally good, do we have the possible to mark as @deprecated for setter and propose to remove since a future version to avoid breaking change on existing usage?
And meanwhile ,for the unit test, can we add more dedicated case on the immutable?

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.

3 participants