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.

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

Reduce HttpProblem dependency on Response.StatusType to the minimum #314

Merged
merged 3 commits into from
Oct 29, 2023

Conversation

lwitkowski
Copy link
Collaborator

@lwitkowski lwitkowski commented Oct 28, 2023

This will allow some certain future features to be much easier to implement, it also reduces non-null checks as we're relying on int status code, not nullable Response.StatusType

return this.statusCode;
}

@Deprecated(forRemoval = true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you @Deprecate something it is best practice to document this in JavaDocs what part of new code replaces deprecated part (even if it is in same class).

I'd suggest also that we should mark this - in JavaDocs - for removal even for next minor version (3.2.0).
In case of community having struggle migrating to new version we can always revert it in 3.2.1.

On the other hand would like to get your point of view why we cannot keep it for developer convinence.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good points. I wanted to get rid of this class from `HttpProblem entirely, but it's still heavily used in the builder by mappers. On the other hand, it will return null for 422 and other non-standard codes.

I'll remove this deprecation now, it's not that critical.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, with this note on null statuses I'm actually in favor of removing it in future.

But agree on not being critical for release.

Copy link
Collaborator

@pazkooda pazkooda left a comment

Choose a reason for hiding this comment

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

I'd prefer to have deprecation reason in code at least.

But beside this detail all other code look good.

@lwitkowski lwitkowski merged commit 6e71fff into master Oct 29, 2023
6 checks passed
@lwitkowski lwitkowski deleted the feat/response-status-decoupling branch October 29, 2023 15:27
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