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

The great Jetty-12 renaming omnibus #9072

Closed
gregw opened this issue Dec 19, 2022 · 12 comments
Closed

The great Jetty-12 renaming omnibus #9072

gregw opened this issue Dec 19, 2022 · 12 comments
Assignees

Comments

@gregw
Copy link
Contributor

gregw commented Dec 19, 2022

Jetty version(s)
12

Enhancement Description

As jetty-12 alpha dev comes to a close, there are few classes and methods that could be renamed. To make our lives simpler, it will be best to do these all at once with minimal other PRs outstanding. So this issue is here to suggest/discuss renamings that we want to do, and when we are all agreed we can do them all at once and quickly merge. Please list suggested renamings in the comments.

@gregw
Copy link
Contributor Author

gregw commented Dec 19, 2022

Handler.Collection to Handler.List or something else? The problem with List is that it will clash with java.util.List in any Handler, be they a List or not. Thus Handler.Sequence is probably better.

@gregw
Copy link
Contributor Author

gregw commented Dec 19, 2022

Handler.process to Handler.handle. This would mean that Handler would not longer implement Request.Processor, but I do not think there are any examples of a Handler being used as just a Request.Processor.

@gregw gregw changed the title The great Jetty-12 renaming The great Jetty-12 renaming omnibus PR Dec 19, 2022
@gregw gregw changed the title The great Jetty-12 renaming omnibus PR The great Jetty-12 renaming omnibus Dec 19, 2022
@gregw
Copy link
Contributor Author

gregw commented Dec 19, 2022

SessionConfig has a TODO for renaming. @janbartel do you know what they are?

@sbordet
Copy link
Contributor

sbordet commented Dec 19, 2022

Handler.process to Handler.handle. This would mean that Handler would not longer implement Request.Processor, but I do not think there are any examples of a Handler being used as just a Request.Processor.

Sure there are! ErrorProcessors!
I remember there was a problem considering them Handlers so I won't mind a common super-interface (maybe separated from Request).

@gregw
Copy link
Contributor Author

gregw commented Dec 19, 2022

@sbordet So an ErrorProcessor is currently just a Processor. It is not a Handler, so would not be affected by the renaming.
Now we might want to go back to an ErrorHandler if we want a life cycle and a server reference in the error thingy, but that would be a different PR as it is more than just a renaming.

I do not think we have any code that is effectively:

    Request.Processor p = myHandler;

and even if we did, that could just become:

    Request.Processor p = myHandler::handle;

@lachlan-roberts
Copy link
Contributor

Adding the websocket isDemanding rename to the omnibus #8991

@gregw
Copy link
Contributor Author

gregw commented Jan 11, 2023

The jetty-core/jetty-ee module is not really ee specific. Currently it just contains the Deployable interface, which can apply to non ee contexts.

It could be renamed to jetty-context-utils or jetty-common and used to hold code that is common between eeN and other types of context.

@gregw
Copy link
Contributor Author

gregw commented Feb 1, 2023

So here is the list of renames that I would like to do:

old name new name Comment
Handler.process Handler.handle Handler would no longer implement Request.Processor
Handler.Nested Handler.Singleton
FrameHandler.isDemanding FrameHandler.isAutoDemand
DecryptedEndPoint SslEndPoint
jetty-core/jetty-ee jetty-core/jetty-context-common This would also be the destination of any common code extracted from the servlet layers
RetainableByteBufferPool ByteBufferPool

I think some of these are trivial, but most will need a PR to be reviewed. But we should do once we have a minimal number of open PRs

@sbordet @janbartel @lorban @lachlan-roberts @joakime @olamy @jmcc0nn3ll

@sbordet
Copy link
Contributor

sbordet commented Feb 7, 2023

Tracking renaming of DecryptedEndPoint to SslEndPoint via #9326.

@gregw
Copy link
Contributor Author

gregw commented Feb 16, 2023

remaining rename to beta1

janbartel added a commit that referenced this issue Jun 8, 2023
* Issue #9072 refactor jetty-ee module to remove it

* Remove old ee imports in module-info.javas

* Remove jetty-ee from deps

* Fix references to jetty-ee for osgi

* Update jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Deployable.java

Co-authored-by: Greg Wilkins <[email protected]>

---------

Co-authored-by: Greg Wilkins <[email protected]>
@janbartel
Copy link
Contributor

@gregw with #9883 completed, can we close this issue now?

@gregw gregw closed this as completed Jun 9, 2023
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Jetty 12.0.0.beta2 Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants