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

Update naming for status codes 413 and 422 #87

Merged

Conversation

Philippus
Copy link
Contributor

@pjfanning
Copy link
Contributor

Are there any tests maintained for the legacy status codes? We need to prove that users can still respond with old status code variant.

@jrudolph
Copy link
Contributor

Nice addition, thanks @Philippus.

Is that something we would consider for 1.0.0? There's little risk but currently I'd say let's postpone to 1.1.x on principle since it is not strictly necessary right now?

@mdedetrich
Copy link
Contributor

Is that something we would consider for 1.0.0? There's little risk but currently I'd say let's postpone to 1.1.x on principle since it is not strictly necessary right now?

Agreed, milestone set

@mdedetrich mdedetrich added this to the 1.1.0 milestone Feb 28, 2023
@mdedetrich
Copy link
Contributor

@Philippus Apologies but my javafmt commit caused some merge conflicts on this PR but they are relatively minor. If you do a rebase on origin/main and run sbt javafmt it should remove the conflicts.

Let me know if you have time/are able to do this, otherwise I can just fix it up before merging the PR.

@pjfanning
Copy link
Contributor

there is no hurry on this as we are unlikely to include this in the pekko-http 1.0.0 release (and add it later instead)

@mdedetrich
Copy link
Contributor

Indeed take your time, its not urgent

@Philippus
Copy link
Contributor Author

I'll fix it up once the first pekko-http is out of the door.

@He-Pin
Copy link
Member

He-Pin commented Aug 9, 2023

@Philippus You need rebase your PR.

@Philippus Philippus force-pushed the update-naming-for-status-codes-413-and-422 branch from 3ff3fa4 to 0c5361d Compare August 9, 2023 21:11
@Philippus Philippus force-pushed the update-naming-for-status-codes-413-and-422 branch from 0c5361d to 5f334dc Compare August 9, 2023 21:39
Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@jrudolph jrudolph left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

Apologies - I was going around approving PRs but I forgot about the Scala 3 compile issue in this PR - hopefully, we;ll get around to fixing that soon

@mdedetrich
Copy link
Contributor

Was about to say that the tests are failing with java.lang.NoClassDefFoundError: Could not initialize class org.apache.pekko.http.scaladsl.model.StatusCodes$

@jrudolph
Copy link
Contributor

Are there any tests maintained for the legacy status codes? We need to prove that users can still respond with old status code variant.

This is only an API change and for those changes we mostly rely on Mima to catch problems. We also have the (currently disabled) "http-compatibility-tests" module which has logic to test backward compatibility in cases which warrant more testing.

Copy link
Contributor

@jrudolph jrudolph left a comment

Choose a reason for hiding this comment

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

Needs the below fixes to fix the runtime errors.

@Philippus Philippus force-pushed the update-naming-for-status-codes-413-and-422 branch from 608f524 to b67169b Compare August 11, 2023 08:38
@Philippus Philippus force-pushed the update-naming-for-status-codes-413-and-422 branch from 37274b3 to 3e5e8ea Compare August 11, 2023 08:39
public static final StatusCode CONTENT_TOO_LARGE =
org.apache.pekko.http.scaladsl.model.StatusCodes.ContentTooLarge();

/** @deprecated deprecated in favor of CONTENT_TOO_LARGE since 1.0.0 */
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be since 1.1.0 - 1.0.0 was released without this change

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good point

@jrudolph
Copy link
Contributor

(btw. test error is likely unrelated)

Copy link
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

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

lgtm

@Philippus Philippus force-pushed the update-naming-for-status-codes-413-and-422 branch from 3e5e8ea to 0bf6079 Compare August 11, 2023 10:39
Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm

@pjfanning pjfanning added the release notes should be mentioned in release notes label Aug 11, 2023
@jrudolph jrudolph merged commit d474334 into apache:main Aug 11, 2023
10 checks passed
@jrudolph
Copy link
Contributor

Thanks again, @Philippus!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes should be mentioned in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants