smartsheet / smartsheet-java-sdk

Library that uses Java to connect to Smartsheet services.
Apache License 2.0
3 stars 12 forks source link

Migrate Some Classes to Use Lombok #78

Closed zromano closed 1 month ago

zromano commented 8 months ago

Migrate some classes that don't have inheritance or an existing builder to use Lombok. These classes previously extended NamedModel<T> or IdentifiableModel<T>. I removed this inheritance and moved the fields back into the class. These were mainly used for overloading the Equals and HashCode methods.

Also updates our PR testing job so that tests will run on any PR, not just ones targeting mainline

Things that will change as a result of this MR:

Things that won't change:

zromano commented 1 month ago

@ronreynolds I finally got around to updating this.

Looks like Jackson doesn't like if we only have builders. It wants a no-arg constructor for deserialization. With that, I think we should go with a default/no-arg constructor and a build for all our models. We can ditch any multi-arg or all-arg constructors.

Thoughts?

ronreynolds commented 1 month ago

@zromano i just checked the openapi-generated models and they too use no-arg ctor; unfortunately they don't use a builder (at least not with the options i used when running the openapi-generator). so their models are still mutable but it's still better than having mixed/all arg ctors IMHO. ah, perhaps i just need to delve into those openapi-generator options further... https://www.baeldung.com/java-openapi-lombok-create-models hmm... not exactly slick (and not quite the annotations i would have gone with) but it's something to consider to be sure...

<configOptions>
    <additionalModelTypeAnnotations>@lombok.Data @lombok.NoArgsConstructor @lombok.AllArgsConstructor</additionalModelTypeAnnotations>
</configOptions>
ronreynolds commented 1 month ago

since this is a breaking (4.0) change it should target a release/4.0 branch, yes, or do we want it going to mainline? and if it does go to mainline does that mean we can't release another 3.x without it?

zromano commented 1 month ago

@ronreynolds I'm targeting release/4.0 which is an unreleased branch with breaking changes. (In hindsight is should've named it future-release/4.0 or something better)

We will still be able to work off mainline and deploy from it.

ronreynolds commented 1 month ago

this build is still generating a 3.2.0 jar; i presume we'll fix that when it gets to the release/4.0 branch? also i noticed in the description "Migrate some classes that don't have inheritance or an existing builder to use Lombok"; as this is intended to be part of a breaking release we should probably revisit those classes that do have builders? as this is part of a not-yet-ready release/4.0 branch i see no reason to not approve it to clear the plate for the next round of changes.

zromano commented 1 month ago

this build is still generating a 3.2.0 jar; i presume we'll fix that when it gets to the release/4.0 branch? also i noticed in the description "Migrate some classes that don't have inheritance or an existing builder to use Lombok"; as this is intended to be part of a breaking release we should probably revisit those classes that do have builders? as this is part of a not-yet-ready release/4.0 branch i see no reason to not approve it to clear the plate for the next round of changes.

I just need to update the version in the build.gradle file to be 4.0.0 instead of 3.2.0. I will do that on my next MR to the 4.0 branch.

Yeah, there will be several more of these MRs to update the remaining model classes. I just picked the easiest ones for this first MR to get our pattern down.