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

Issue #5401 - Moving jetty-http-tools top level project. #5402

Closed
wants to merge 4 commits into from

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Oct 6, 2020

  • Ensures that it will be deployed.
  • Adding to generated (aggregate-like) javadoc as well.

Signed-off-by: Joakim Erdfelt joakim.erdfelt@gmail.com

+ Ensures that it will be deployed.
+ Adding to generated (aggregate-like) javadoc as well.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime self-assigned this Oct 6, 2020
@joakime joakime added the Task label Oct 6, 2020
@joakime joakime linked an issue Oct 6, 2020 that may be closed by this pull request
+ Updating jetty-http-tools dependencies, as it's no longer
  in the org.eclipse.jetty.tests groupId.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
requires transitive org.eclipse.jetty.http;

// Optional - only required if you use hamcrest too.
requires static org.hamcrest;
Copy link
Contributor

Choose a reason for hiding this comment

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

OMG. Just to support assertThat() now we need to depend on an automatic module.
Can we just get rid of the hamcrest dependency? It adds nothing and just complicates things -- everything is already available in the HttpFields API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into that (getting rid of hamcrest in jetty-http-tools), but it made for a MUCH larger PR to change the existing code (in the rest of jetty) that uses these hamcrest features exposed in jetty-http-tools.
If you are gung-ho on this, I'll start work on removing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the hamcrest style!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, will do.
I'll rebuild this PR from scratch with the jetty-http-tools split between top level and matchers still in /tests/

+ Moved far worse description-less test assertions.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Copy link
Contributor Author

joakime commented Oct 7, 2020

Will rebuild in cleaner way.

@joakime joakime closed this Oct 7, 2020
@joakime joakime deleted the jetty-10.0.x-5401-move-jetty-http-tools branch October 8, 2020 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move jetty-http-tools under the project root
3 participants