Skip to content

Conversation

M-Faheem-Khan
Copy link

@M-Faheem-Khan M-Faheem-Khan commented Apr 26, 2025

Context

Remove deprecated code as well as associated tests.

Change

Cookie

  • Remove Comment method.
  • Remove Version method.

SessionCookie

  • Remove Comment method.
  • Remove Version method.

HTTPServlet

  • Remove LEGACY_DO_HEAD.
  • Remove PushBuilder

Validation

  • Tests Pass
  • Successfully Compiles

Environment

Java: 17.0.9-amzn
Maven: 3.9.9

image

See attached results of mvn install command
mvn-install-output.txt

Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
@markt-asf
Copy link
Contributor

Removing deprecated code requires a new major version.

Also PushBuilder is not currently marked for removal. Early hints support has only just been added for 6.2 so we can't remove PushBuilder just yet.

I would be in favour of updating PushBuilder's deprecation to mark it for removal with the intention to remove it in 7.0.

The next release is planned to be 6.2 not 7.0. We could update the plan but I'm not seeing a reason to spend time doing that rather than working through the currently open issues. Particularly given that if we switched to 7.0 now we could only remove the deprecated Cookie methods. PushBuilder would need to wait until 8.0. I'd rather do 6.2 now and remove both in a future 7.0.

@M-Faheem-Khan
Copy link
Author

M-Faheem-Khan commented Apr 26, 2025

I didn't realize the PushBuilder isn't ready to be removed. I've made change following your comment. Let me know if there is missing or needs to be changed. Ty

@markt-asf
Copy link
Contributor

My previous comments stand. I think it is too early to remove anything. I also think the PushBuilder deprecation should be updated to "for removal".

Signed-off-by: M-Faheem-Khan <faheem5948@gmail.com>
@M-Faheem-Khan M-Faheem-Khan closed this by deleting the head repository May 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants