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

docs: event stream typed document (#29109) #32100

Merged
merged 11 commits into from
Oct 10, 2023

Conversation

Roiocam
Copy link
Contributor

@Roiocam Roiocam commented Sep 14, 2023

References #29109

@Roiocam Roiocam marked this pull request as draft September 14, 2023 16:09
@Roiocam Roiocam closed this Sep 16, 2023
@Roiocam Roiocam reopened this Sep 16, 2023
@Roiocam
Copy link
Contributor Author

Roiocam commented Sep 16, 2023

close-reopen to retrigger paused CI.

Copy link
Contributor Author

@Roiocam Roiocam left a comment

Choose a reason for hiding this comment

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

PR passed. Let's take a look at these typed cases to see if they are correct.

@Roiocam Roiocam marked this pull request as ready for review September 16, 2023 20:12
@Roiocam Roiocam requested a review from johanandren September 25, 2023 09:54
Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

Looking good, I left some smaller feedback

akka-docs/src/main/paradox/typed/event-stream.md Outdated Show resolved Hide resolved
akka-docs/src/main/paradox/typed/event-stream.md Outdated Show resolved Hide resolved
Comment on lines 47 to 57
@@@ div { .group-java }

@@snip [LoggingDocTest.java](/akka-actor-typed-tests/src/test/java/akka/actor/typed/eventstream/LoggingDocTest.java) { #imports-deadletter }

@@snip [LoggingDocTest.java](/akka-actor-typed-tests/src/test/java/akka/actor/typed/eventstream/LoggingDocTest.java) { #deadletter-actor }

it can be subscribed like this:

@@snip [LoggingDocTest.java](/akka-actor-typed-tests/src/test/java/akka/actor/typed/eventstream/LoggingDocTest.java) { #deadletters }

@@@
Copy link
Member

Choose a reason for hiding this comment

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

The div { group-java/scala } blocks doesn't lead to a tabbed code box, which is especially unfortunate as. a first code snippet, better turn them into complete regular code snippet blocks and either have the text "it can be subscribed like this" inbetween for both Scala and Java with a second code snippet for subscribing for both languages, or put it all in one code block and skip that text.

Suggested change
@@@ div { .group-java }
@@snip [LoggingDocTest.java](/akka-actor-typed-tests/src/test/java/akka/actor/typed/eventstream/LoggingDocTest.java) { #imports-deadletter }
@@snip [LoggingDocTest.java](/akka-actor-typed-tests/src/test/java/akka/actor/typed/eventstream/LoggingDocTest.java) { #deadletter-actor }
it can be subscribed like this:
@@snip [LoggingDocTest.java](/akka-actor-typed-tests/src/test/java/akka/actor/typed/eventstream/LoggingDocTest.java) { #deadletters }
@@@
Java
: @@snip [LoggingDocTest.java](/akka-actor-typed-tests/src/test/java/akka/actor/typed/eventstream/LoggingDocTest.java) { #imports-deadletter #deadletter-actor #deadletters }

Copy link
Contributor Author

@Roiocam Roiocam Oct 10, 2023

Choose a reason for hiding this comment

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

You are correct, however, this div { group-java/scala } block will still follow tab switching.

Therefore, we will add a Tabbed to the first snippet of this block and retain the div { group-java } only, as it is not allowed to combine imports and class definitions in Java (it would be broken by the format plugin).

I have pushed a commit to explain this, and the final result is like this on my PC, I think this will be better. :)

截屏2023-10-10 10 10 54

Copy link
Member

Choose a reason for hiding this comment

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

Ok, that is better, the big problem was the first code snippet in the page not having the tabs.

akka-docs/src/main/paradox/typed/event-stream.md Outdated Show resolved Hide resolved
@Roiocam Roiocam requested a review from johanandren October 10, 2023 02:38
Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@johanandren johanandren merged commit 7cb2ff7 into akka:main Oct 10, 2023
4 checks passed
@Roiocam Roiocam deleted the typed-event-stream-document branch October 10, 2023 07:32
He-Pin pushed a commit to He-Pin/akka that referenced this pull request Jan 7, 2024
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