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

Use SELF for Builder Self Type Parameter #5684

Open
minwoox opened this issue May 17, 2024 · 1 comment
Open

Use SELF for Builder Self Type Parameter #5684

minwoox opened this issue May 17, 2024 · 1 comment
Labels
improvement

Comments

@minwoox
Copy link
Contributor

minwoox commented May 17, 2024

In the past, we decided not to use the self type parameter in this PR because it resulted in poorly formatted Javadoc. However, this decision has introduced several problems:

  • Cumbersome Overrides: Developers are required to override numerous methods that add little value, which is cumbersome and error-prone.
  • Deprecated Annotation Issues: The @Deprecated annotation is not propagated to overridden methods, increasing the likelihood of mistakes.
  • Type Errors: There is a high chance of developers making mistakes by using the wrong type, as evidenced in this example.
  • Benchmarking Concerns: Service developers who benchmark Armeria code use the same approach, even though Javadoc formatting should not be a concern for them.

Moreover, with the current JDK, Javadoc is generated correctly with type parameters. Therefore, I propose using SELF for builder self type parameters to address these issues.
minwoox added a commit to minwoox/armeria that referenced this issue May 17, 2024
@minwoox
Use SELF for Builder Self Type Parameter
d649678 
Motivation:
See line#5684 for the motivation.

Modifications:
- Refactored builder classes to use the SELF type parameter for the builder's self-referencing methods.
  - Didn't refactor builder classes if their parent classes are public due to breaking changes. Will revisit this later if making change is acceptable.

Result:
- Reduced the need for cumbersome and error-prone method overrides.
@minwoox minwoox mentioned this issue May 17, 2024
Merged
minwoox added a commit that referenced this issue Jun 5, 2024
@minwoox
Use SELF for Builder Self Type Parameter (#5685)
fb9132a 
Motivation:
See #5684 for the motivation.

Modifications:
- Refactored builder classes to use the SELF type parameter for the builder's self-referencing methods.
  - Didn't refactor builder classes if their parent classes are public due to breaking changes. Will revisit this later if making changes is acceptable.

Result:
- Reduced the need for cumbersome and error-prone method overrides.
minwoox added a commit to minwoox/armeria that referenced this issue Jun 5, 2024
@minwoox
Use SELF for Builder Self Type Parameter (part2)
af43347 
Motivation:
See line#5684 for the motivation.

Modifications:
- Refactored builder classes to use the SELF type parameter for the builder's self-referencing methods.

Result:
- Reduced the need for cumbersome and error-prone method overrides.
@minwoox minwoox mentioned this issue Jun 5, 2024
Merged
@minwoox
Copy link
Contributor Author

minwoox commented Jun 5, 2024

The list of classes that we should fix when we reach 2.0

AbstractHttpMessageBuilder
AbstractCircuitBreakerClientBuilder
AbstractClientOptionsBuilder
AbstractRetryingClientBuilder
AbstractRequestContextBuilder
AbstractMetricCollectingBuilder
AbstractJettyServiceBuilder
AbstractHttpFileBuilder
LoggingDecoratorBuilder

trustin pushed a commit that referenced this issue Jun 7, 2024
@minwoox
Use SELF for Builder Self Type Parameter (part2) (#5733)
d4d72aa 
Motivation:
See #5684 for the motivation.

Modifications:
- Refactored builder classes to use the SELF type parameter for the
builder's self-referencing methods.

Result:
- Reduced the need for cumbersome and error-prone method overrides.
- (Breaking) These are all annotated with `@UnstableApi`.
- `AbstractDnsResolverBuilder` is now `AbstractDnsResolverBuilder<SELF>`
  - `AbstractRuleBuilder` is now `AbstractRuleBuilder<SELF>`
- `AbstractRuleWithContentBuilder` is now
`AbstractRuleWithContentBuilder<SELF>`
- `AbstractCircuitBreakerMappingBuilder` is now
`AbstractCircuitBreakerMappingBuilder<SELF>`
- `AbstractDynamicEndpointGroupBuilder` is now
`AbstractDynamicEndpointGroupBuilder<SELF>`
- `DynamicEndpointGroupSetters` is now
`DynamicEndpointGroupSetters<SELF>`
- `AbstractCuratorFrameworkBuilder` is now
`AbstractCuratorFrameworkBuilder<SELF>`
Dogacel pushed a commit to Dogacel/armeria that referenced this issue Jun 8, 2024
@minwoox @Dogacel
Use SELF for Builder Self Type Parameter (line#5685)
cc320bf 
Motivation:
See line#5684 for the motivation.

Modifications:
- Refactored builder classes to use the SELF type parameter for the builder's self-referencing methods.
  - Didn't refactor builder classes if their parent classes are public due to breaking changes. Will revisit this later if making changes is acceptable.

Result:
- Reduced the need for cumbersome and error-prone method overrides.
Dogacel pushed a commit to Dogacel/armeria that referenced this issue Jun 8, 2024
@minwoox @Dogacel
Use SELF for Builder Self Type Parameter (part2) (line#5733)
cfdd4b5 
Motivation:
See line#5684 for the motivation.

Modifications:
- Refactored builder classes to use the SELF type parameter for the
builder's self-referencing methods.

Result:
- Reduced the need for cumbersome and error-prone method overrides.
- (Breaking) These are all annotated with `@UnstableApi`.
- `AbstractDnsResolverBuilder` is now `AbstractDnsResolverBuilder<SELF>`
  - `AbstractRuleBuilder` is now `AbstractRuleBuilder<SELF>`
- `AbstractRuleWithContentBuilder` is now
`AbstractRuleWithContentBuilder<SELF>`
- `AbstractCircuitBreakerMappingBuilder` is now
`AbstractCircuitBreakerMappingBuilder<SELF>`
- `AbstractDynamicEndpointGroupBuilder` is now
`AbstractDynamicEndpointGroupBuilder<SELF>`
- `DynamicEndpointGroupSetters` is now
`DynamicEndpointGroupSetters<SELF>`
- `AbstractCuratorFrameworkBuilder` is now
`AbstractCuratorFrameworkBuilder<SELF>`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
improvement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@minwoox