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

Reworking jetty-compression for JPMS #12531

Merged
merged 27 commits into from
Nov 18, 2024

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Nov 13, 2024

A quick reworking of jetty-compression for JPMS.

@joakime joakime added the Bug For general bugs on Jetty side label Nov 13, 2024
@joakime joakime requested a review from sbordet November 13, 2024 19:35
@joakime joakime self-assigned this Nov 13, 2024
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

I would also move to internal:

  • oejc.server.CompressionResponse
  • oejc.server.DecompressionRequest

Please also have a run across all the modules to fix simple warnings such as fields not used (e.g. CompressionResponse.LOG), fields that can be final, parameters not used (e.g. DecompressionRequest constructor), GzipCompression, etc.

@joakime joakime requested a review from sbordet November 14, 2024 20:21
@sbordet
Copy link
Contributor

sbordet commented Nov 15, 2024

Please also change all public static final constant to be private.

@joakime joakime requested a review from sbordet November 15, 2024 20:02
@joakime joakime requested a review from sbordet November 18, 2024 17:07
@@ -178,7 +178,7 @@ public Set<String> getCompressIncludeMimeTypes()
* @see #getCompressExcludePaths()
*/
@ManagedAttribute("Set of Response Compression Path Exclusions")
public Set<String> getCompressPathIncludes()
public Set<String> getCompressIncludesPath()
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to getCompressIncludesPaths(), plural like the others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doh! bad plural. :-/

@sbordet sbordet self-requested a review November 18, 2024 20:30
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

Fix @ManagedAttribute typos.

@gregw gregw self-requested a review November 18, 2024 20:39
@sbordet sbordet self-requested a review November 18, 2024 20:43
@joakime joakime merged commit f480f46 into jetty-12.1.x Nov 18, 2024
2 of 8 checks passed
@joakime joakime deleted the fix/12.1.x/compression-module-info branch November 18, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants