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

Adds synchronization of resources for refresh mojo #478

Conversation

abelsromero
Copy link
Member

Thank you for opening a pull request and contributing to asciidoctor-maven-plugin!

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Documentation
  • Refactor
  • Build improvement
  • Other (please describe)

What is the goal of this pull request?
Adds efficient synchronization of resources for refresh mojo.

Are there any alternative ways to implement this?
*FileAlterationListenerAdaptor hierarchy has some duplication but its a tradeoff I think is worth. The other alterntive is making some complex solutions with several lambdas or create abstract methods to customize log messages.

Are there any implications of this pull request? Anything a user must know?
Closes #472

Is it related to an existing issue?

  • Yes
  • No

Finally, please add a corresponding entry to CHANGELOG.adoc

@abelsromero abelsromero added this to the 2.0.x milestone Jul 30, 2020
@abelsromero abelsromero changed the title Adds synchronization of resources for refresh mojo WIP: Adds synchronization of resources for refresh mojo Jul 30, 2020
@coveralls
Copy link

coveralls commented Jul 30, 2020

Coverage Status

Coverage increased (+2.1%) to 82.579% when pulling 73d1540 on abelsromero:feature/synchronize-resources-in-refresh-mojo into ae49ced on asciidoctor:master.

@abelsromero abelsromero force-pushed the feature/synchronize-resources-in-refresh-mojo branch 2 times, most recently from 5acef2d to 4c1883b Compare August 1, 2020 18:43
@abelsromero abelsromero force-pushed the feature/synchronize-resources-in-refresh-mojo branch from 4c1883b to 9a11bc1 Compare August 1, 2020 18:49
@abelsromero
Copy link
Member Author

abelsromero commented Aug 1, 2020

Side note, with the new tests, build time has increased considerably

@abelsromero abelsromero force-pushed the feature/synchronize-resources-in-refresh-mojo branch from e879b85 to 254af74 Compare August 2, 2020 17:41
@abelsromero abelsromero changed the title WIP: Adds synchronization of resources for refresh mojo Adds synchronization of resources for refresh mojo Aug 3, 2020
@abelsromero
Copy link
Member Author

PR ready!

@abelsromero
Copy link
Member Author

The problem with coverage reports does not seem to be a random thing. A workarround is available, but we should try to see the real reason.
jacoco/jacoco#1076

@abelsromero abelsromero force-pushed the feature/synchronize-resources-in-refresh-mojo branch from d1d1e05 to 538a965 Compare August 4, 2020 14:47
@abelsromero
Copy link
Member Author

Fixed the issues with jacoco. It required some changes in the http-mojo.
Just applied minimal changes to make it work properly. We can improve it in another PR.

@abelsromero abelsromero force-pushed the feature/synchronize-resources-in-refresh-mojo branch from 538a965 to 6471cd1 Compare August 4, 2020 15:09
@abelsromero abelsromero force-pushed the feature/synchronize-resources-in-refresh-mojo branch from 6471cd1 to 73d1540 Compare August 4, 2020 18:45
@abelsromero
Copy link
Member Author

+3 % coverage 🥳

@abelsromero abelsromero merged commit 3b78659 into asciidoctor:master Aug 4, 2020
@abelsromero abelsromero deleted the feature/synchronize-resources-in-refresh-mojo branch August 4, 2020 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support auto-refresh with minimal features to be usefull
2 participants