-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Fixes #10293 - Improve documentation on how to write a response body … #10434
Conversation
…in Jetty 12. Updated documentation about: * Content.Source * Content.Sink * Handler * Request/Response Updated few APIs to make easier to write applications. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
* @return a new {@link Completable} passed to the consumer | ||
* @see #with(Consumer) | ||
*/ | ||
public Completable compose(Consumer<Completable> consumer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a change that's bigger than just documentation.
Fields formFields = FormFields.from(request).get(); | ||
return Fields.combine(queryFields, formFields); | ||
CompletableFuture<Fields> contentFields = FormFields.from(request); | ||
return contentFields.thenApply(formFields -> Fields.combine(queryFields, formFields)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another change that isn't just documentation? is this what broke the tests in this latest build?
* @param charset the charset to use to decode bytes | ||
* @return the {@link CompletableFuture} to notify when the whole content has been read | ||
*/ | ||
static CompletableFuture<String> asStringAsync(Source source, Charset charset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another non-documentation change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is bigger than "just documentation".
Some niggles about @Deprecated
use.
@@ -25,7 +25,9 @@ public class HttpStatus | |||
public static final int CONTINUE_100 = 100; | |||
public static final int SWITCHING_PROTOCOLS_101 = 101; | |||
public static final int PROCESSING_102 = 102; | |||
@Deprecated(forRemoval = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also have since
...
@Deprecated(forRemoval = true) | |
@Deprecated(forRemoval = true, since = "12.0.1") |
@@ -104,7 +106,9 @@ public enum Code | |||
CONTINUE(CONTINUE_100, "Continue"), | |||
SWITCHING_PROTOCOLS(SWITCHING_PROTOCOLS_101, "Switching Protocols"), | |||
PROCESSING(PROCESSING_102, "Processing"), | |||
@Deprecated(forRemoval = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Deprecated(forRemoval = true) | |
@Deprecated(forRemoval = true, since = "12.0.1" |
@@ -123,7 +127,7 @@ public enum Code | |||
NOT_MODIFIED(NOT_MODIFIED_304, "Not Modified"), | |||
USE_PROXY(USE_PROXY_305, "Use Proxy"), | |||
TEMPORARY_REDIRECT(TEMPORARY_REDIRECT_307, "Temporary Redirect"), | |||
// Keeping the typo for backward compatibility for a while | |||
@Deprecated(forRemoval = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Deprecated(forRemoval = true) | |
@Deprecated(forRemoval = true, since = "12.0.1" |
…in Jetty 12.
Updated documentation about:
Updated few APIs to make easier to write applications.