Skip to content

ice: Use classes to create describe metadata instead of stringBuilder.#21

Merged
shyiko merged 7 commits intomasterfrom
replace_stringbuilder_describe
Apr 29, 2025
Merged

ice: Use classes to create describe metadata instead of stringBuilder.#21
shyiko merged 7 commits intomasterfrom
replace_stringbuilder_describe

Conversation

@subkanthi
Copy link
Collaborator

@subkanthi subkanthi commented Apr 24, 2025

closes: #19

import com.fasterxml.jackson.annotation.JsonInclude;
import java.util.List;
import java.util.Map;
import lombok.Data;
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we use standard records instead of pulling in lombok? looks like jackson already supports them

Copy link
Collaborator Author

@subkanthi subkanthi Apr 24, 2025

Choose a reason for hiding this comment

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

can we use standard records instead of pulling in lombok? looks like jackson already supports them,

lombok is for adding setter/getter , unfortunately data is misleading.

https://projectlombok.org/features/Data
@DaTa is a convenient shortcut annotation that bundles the features of @ToString, @EqualsAndHashCode, @Getter / @Setter and @RequiredArgsConstructor together: In other words, @DaTa generates all the boilerplate that is normally associated with simple POJOs (Plain Old Java Objects) and beans: getters for all fields, setters for all non-final fields, and appropriate toString, equals and hashCode implementations that involve the fields of the class, and a constructor that initializes all final fields, as well as all non-final fields with no initializer that have been marked with @nonnull, in order to ensure the field is never null.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I get that but we don't really care about any of that in this case. All we need is something to holds the data before it gets converted to yaml/json. A Map would work, but records would be even better. Pulling lombok when both Maps and records would suffice feels excessive.

@subkanthi subkanthi changed the title Use classes to create describe metadata instead of stringBuilder. ice: Use classes to create describe metadata instead of stringBuilder. Apr 24, 2025
Copy link
Collaborator

@shyiko shyiko left a comment

Choose a reason for hiding this comment

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

❤️

@subkanthi subkanthi marked this pull request as ready for review April 25, 2025 20:13
// TODO: refactor: the use of StringBuilder below is absolutely criminal
public static void run(RESTCatalog catalog, String target, boolean json, boolean includeMetrics)
throws IOException {
public static void run(RESTCatalog catalog, String target, boolean json) throws IOException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like TableMetadata, etc. records are defined but not actually used...

@shyiko shyiko merged commit 85940bb into master Apr 29, 2025
1 check passed
@shyiko shyiko deleted the replace_stringbuilder_describe branch June 3, 2025 18:17
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.

ice: Refactor describe to use types instead of sb to generate output

2 participants