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.

Sign up for GitHub

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

Jump to bottom

DBZ-6835 Documentation for Timezone SMT #4819

Merged
merged 4 commits into from
Sep 8, 2023
Merged

DBZ-6835 Documentation for Timezone SMT #4819

merged 4 commits into from
Sep 8, 2023

Conversation

ani-sha
Copy link
Member

@ani-sha ani-sha commented Aug 31, 2023

@ani-sha ani-sha mentioned this pull request Aug 28, 2023
Merged
3 tasks
Naros
Naros requested changes Aug 31, 2023
View reviewed changes
Copy link
Member

@Naros Naros left a comment

Choose a reason for hiding this comment

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

I've added a few suggestions.

A general suggestion, however, is that we should strive only to have a single sentence per line in the documentation. We haven't been consistent with this universally, but it is the dominant style we've used, and I'd prefer to stick to that.

documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
@roldanbob
Copy link
Contributor

roldanbob commented Aug 31, 2023 •
edited

I've added a few suggestions...

@ani-sha I added my review. To reduce the confusion that might result in trying to integrate possibly divergent suggestions, before submitting my review, I first went through the comments that @Naros added and made a few in-place edits to those (So if anything looks different, Chris, that's why). I then added any additional feedback that I had. Let me know if you have any questions, particularly around my proposed reformatting of the examples. Thanks!

roldanbob
roldanbob requested changes Sep 1, 2023
View reviewed changes
documentation/modules/ROOT/pages/transformations/index.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
@ani-sha
Copy link
Member Author

ani-sha commented Sep 1, 2023

Appreciate your review, @roldanbob. I've incorporated Chris's revisions initially and then integrated your suggested changes. If anything is unclear, please let me know.

roldanbob
roldanbob requested changes Sep 1, 2023
View reviewed changes
Copy link
Contributor

@roldanbob roldanbob left a comment

Choose a reason for hiding this comment

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

A few further suggestions, @ani-sha

documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
@Naros
Copy link
Member

Naros commented Sep 5, 2023

@ani-sha LGTM, this can safely be merged when the TimestampConverter SMT code PR is merged.

@Naros Naros added the waits for other PR Issues that can only be merged once another PR has been merged label Sep 5, 2023
jpechane
jpechane approved these changes Sep 6, 2023
View reviewed changes
Copy link
Contributor

@jpechane jpechane left a comment

Choose a reason for hiding this comment

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

LGTM!

roldanbob
roldanbob requested changes Sep 6, 2023
View reviewed changes
Copy link
Contributor

@roldanbob roldanbob left a comment

Choose a reason for hiding this comment

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

@ani-sha One more set of suggestions. After this, I think you're good to go. Let me know if you have questions.

documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
documentation/modules/ROOT/pages/transformations/timezone-converter.adoc Outdated Show resolved Hide resolved
roldanbob
roldanbob requested changes Sep 7, 2023
View reviewed changes
Copy link
Contributor

@roldanbob roldanbob left a comment

Choose a reason for hiding this comment

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

I responded to your comments and made one further change to the title of the first example in Effect of applying the TZ converter SMT. Beyond that I don't have any further changes to suggest.
I am OOO for the next 10 days. If you have further questions that can wait until after I return, I'll respond when I'm back.

@ani-sha
Copy link
Member Author

ani-sha commented Sep 8, 2023

Thanks a lot @roldanbob! I have added all the suggestions. Looks good now!

@ani-sha ani-sha removed the waits for other PR Issues that can only be merged once another PR has been merged label Sep 8, 2023
@ani-sha
Copy link
Member Author

ani-sha commented Sep 8, 2023

@jpechane the docs are ready to be merged!

@Naros
Copy link
Member

Naros commented Sep 8, 2023

Thanks @ani-sha and @roldanbob for the work on this, applied to main (=2.4).

@Naros Naros merged commit 9ce0e7d into debezium:main Sep 8, 2023
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roldanbob roldanbob roldanbob requested changes

@jpechane jpechane jpechane approved these changes

@Naros Naros Naros approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@ani-sha @roldanbob @Naros @jpechane