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

Get string representation of javadsl Content and Media Type #630

Merged

Conversation

mdedetrich
Copy link
Contributor

@mdedetrich mdedetrich commented Nov 15, 2024

Currently in the javadsl its not possible to get the string representation of the Content-Type/Media-Type because its only the scaladsl that implements the value function (via the ValueRenderable trait). This PR adds the value method to the core Java interfaces (the render method is anyways implemented in the subclasses hence why this PR is compiling without any additional changes)

As a workaround I currently have to do this (its in kotlin but should be understandable)

internal fun contentTypeToString(contentType: ContentType): String {
    return when (contentType) {
        is org.apache.pekko.http.scaladsl.model.ContentType ->
            return contentType.value()
        else -> throw RuntimeException("Unsupported content type: ${contentType::class.qualifiedName}")
    }
}

This works because it just so happens that every javdsl ContentType is also a scaladsl ContentType

@mdedetrich mdedetrich force-pushed the content-type-media-type-string-render branch 2 times, most recently from bb3e7b4 to 125cc72 Compare November 15, 2024 13:05
@mdedetrich mdedetrich added this to the 1.2.0 milestone Nov 15, 2024
@mdedetrich mdedetrich force-pushed the content-type-media-type-string-render branch from 125cc72 to e810ad7 Compare November 15, 2024 13:16
Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm - the 1.1.x branch exists now so this is safe to merge but I would appreciate a delay on the merge until we get #631 merged

@mdedetrich
Copy link
Contributor Author

lgtm - the 1.1.x branch exists now so this is safe to merge but I would appreciate a delay on the merge until we get #631 merged

I would wait for 1.2.x, there is a workaround and since we are adding methods to an interface we should be bumping the minor version

@pjfanning
Copy link
Contributor

lgtm - the 1.1.x branch exists now so this is safe to merge but I would appreciate a delay on the merge until we get #631 merged

I would wait for 1.2.x, there is a workaround and since we are adding methods to an interface we should be bumping the minor version

  • we use 'main' branch for the development version and 1.2.0 is becoming the development version
  • future 1.1.x patch releases will be built using the '1.1.x' branch

@raboof
Copy link
Member

raboof commented Nov 26, 2024

We should probably edit CustomMediaTypesExampleTest to use the new .value() instead of the current .toString().

@mdedetrich
Copy link
Contributor Author

mdedetrich commented Nov 27, 2024

We should probably edit CustomMediaTypesExampleTest to use the new .value() instead of the current .toString().

Done

@mdedetrich mdedetrich force-pushed the content-type-media-type-string-render branch from e810ad7 to 1046b5d Compare November 27, 2024 09:46
@mdedetrich mdedetrich force-pushed the content-type-media-type-string-render branch from 1046b5d to 6e2888c Compare November 27, 2024 10:08
@mdedetrich
Copy link
Contributor Author

@pjfanning Can you re-review this PR given @raboof 's comments?

@mdedetrich mdedetrich requested a review from pjfanning November 27, 2024 10:12
@pjfanning
Copy link
Contributor

@pjfanning Can you re-review this PR given @raboof 's comments?

The changes look ok. We still need to block the merge until we have the 1.1.x branch and related CI builds.

@raboof
Copy link
Member

raboof commented Nov 27, 2024

We still need to block the merge until we have the 1.1.x branch and related CI builds.

I think I missed why we'd want to delay this merge - this PR as it now stands seems like a regular backwards-compatible change (assuming nobody inherited from ContentType or MediaType, which seems like a reasonable assumption)?

@pjfanning
Copy link
Contributor

We still need to block the merge until we have the 1.1.x branch and related CI builds.

I think I missed why we'd want to delay this merge - this PR as it now stands seems like a regular backwards-compatible change (assuming nobody inherited from ContentType or MediaType, which seems like a reasonable assumption)?

The PR currently has @since 1.2.0. If these change to @since 1.1.1 then we don't need to start preparing for 1.2.0 snapshots.

@raboof
Copy link
Member

raboof commented Nov 27, 2024

We still need to block the merge until we have the 1.1.x branch and related CI builds.

I think I missed why we'd want to delay this merge - this PR as it now stands seems like a regular backwards-compatible change (assuming nobody inherited from ContentType or MediaType, which seems like a reasonable assumption)?

The PR currently has @since 1.2.0. If these change to @since 1.1.1 then we don't need to start preparing for 1.2.0 snapshots.

Aah, gotcha. This begs the question whether those @since annotations are really worth having. I can easily imagine situations where it isn't clear yet whether the next release will be a minor, patch or major one. Should/have we discuss(ed) this on the mailinglist?

@pjfanning
Copy link
Contributor

We still need to block the merge until we have the 1.1.x branch and related CI builds.

I think I missed why we'd want to delay this merge - this PR as it now stands seems like a regular backwards-compatible change (assuming nobody inherited from ContentType or MediaType, which seems like a reasonable assumption)?

The PR currently has @since 1.2.0. If these change to @since 1.1.1 then we don't need to start preparing for 1.2.0 snapshots.

Aah, gotcha. This begs the question whether those @since annotations are really worth having. I can easily imagine situations where it isn't clear yet whether the next release will be a minor, patch or major one. Should/have we discuss(ed) this on the mailinglist?

I guess we can discuss the philosophies around using since annotations on the mailing list - I would strongly argue that we can't change the established approach in a patch release.

@raboof
Copy link
Member

raboof commented Nov 27, 2024

We still need to block the merge until we have the 1.1.x branch and related CI builds.

I think I missed why we'd want to delay this merge - this PR as it now stands seems like a regular backwards-compatible change (assuming nobody inherited from ContentType or MediaType, which seems like a reasonable assumption)?

The PR currently has @since 1.2.0. If these change to @since 1.1.1 then we don't need to start preparing for 1.2.0 snapshots.

Aah, gotcha. This begs the question whether those @since annotations are really worth having. I can easily imagine situations where it isn't clear yet whether the next release will be a minor, patch or major one. Should/have we discuss(ed) this on the mailinglist?

I guess we can discuss the philosophies around using since annotations on the mailing list

OK, I can start that thread.

I would strongly argue that we can't change the established approach in a patch release.

OK. AFAICS the 1.1.x branch has already been created, meaning the @since 1.2.0 is correct, right? Is there anything that still needs to be done before #631 can be un-drafted and merged?

@mdedetrich
Copy link
Contributor Author

mdedetrich commented Nov 27, 2024

I think I missed why we'd want to delay this merge - this PR as it now stands seems like a regular backwards-compatible change (assuming nobody inherited from ContentType or MediaType, which seems like a reasonable assumption)?

So actually, the reason for putting it in 1.2.0 is that while its not a breaking compatible change it is a forwards breaking change (hence the MiMa changes) which is allowed by semver in minor version bumps and not patch (its only backwards compatibility thats disallowed, this has to be done in a major patch update).

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

approved for main branch merge - now that main branch is repurposed for v1.2 dev

@mdedetrich
Copy link
Contributor Author

Thanks, I will go ahead and merge it.

@mdedetrich mdedetrich merged commit e7bd97a into apache:main Nov 27, 2024
10 checks passed
@mdedetrich mdedetrich deleted the content-type-media-type-string-render branch November 27, 2024 14:07
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.

5 participants