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

Move underlying code of publishToConfluence into class #1397

Merged
merged 1 commit into from
May 31, 2024

Conversation

seflue
Copy link

@seflue seflue commented May 4, 2024

What changed

The main part of publishToConfluence was executed in a Groovy script, which was evaluated from the Gradle task.

To increase developability of the feature, move the code from the script into an actual Groovy class and call it from the Gradle task.

Motivation

I create this as a draft PR. If you relate with the idea, please give me a hint. I then would actually use the opportunity to make the new class a bit more testable and add some tests.

Because I didn't found any tests for publishToConfluence, I just tested it out with a testspace in the Confluence Cloud instance I have access to. How do you actually tested this in the past? Is there a collection of fixtures I missed?

The motivation behind this PR is, that I am going to use DocToolChain quite often in the upcoming future (especially the "publishToConfluence" feature). I learned about it in a workshop @rdmueller gave last year and introduced it in my current project. Because I love to improve the tools I am using, I would like to contribute to DocToolChain.

Copy link

netlify bot commented May 4, 2024

Deploy Preview for dtc-docs-preview ready!

Name Link
🔨 Latest commit 6c0b6fb
🔍 Latest deploy log https://app.netlify.com/sites/dtc-docs-preview/deploys/6659bab6aa35bb0008196b58
😎 Deploy Preview https://deploy-preview-1397--dtc-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@rdmueller rdmueller left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I guess @PacoVK should also have a look at it.

@rdmueller
Copy link
Member

thanx for this contribution!

@seflue seflue marked this pull request as ready for review May 31, 2024 00:17
@seflue
Copy link
Author

seflue commented May 31, 2024

@rdmueller If you don't mind, you can merge now. When I have time to write some tests, I will create another PR. But for me, the changes just work fine.

@PacoVK
Copy link
Collaborator

PacoVK commented May 31, 2024

Thanks for the good work! I will take a look and then merge it. Apparently I waited because of the draft mode so far 😊🖖

Copy link
Collaborator

@PacoVK PacoVK left a comment

Choose a reason for hiding this comment

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

One remark and then we are good to go, good job👏
Please feel free to put you into the contributors file as well

scripts/publishToConfluence.gradle Outdated Show resolved Hide resolved
The main part of publishToConfluence was executed in an groovy script, which was
evaluated from the Gradle task.

To increase developability of the feature, move the code from the script
into an actual Groovy class and call it from the Gradle task.
@seflue seflue force-pushed the refactor/asciidoc2confluence branch from 0542e5e to 6c0b6fb Compare May 31, 2024 11:55
@PacoVK PacoVK merged commit ec9b20c into docToolchain:ng May 31, 2024
10 checks passed
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.

None yet

3 participants