-
-
Notifications
You must be signed in to change notification settings - Fork 642
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
Change writers to be performed on a per-documentation-set basis #3496
Conversation
We are inching closer to supporting the whole documentation pipeline per documentation set instead of a per-project level. This is one of the final steps to make the architecture work in this way. Once this is solved, we can start experimenting with enabling multi-documentation-set rendering and discovering the issues stemming from that.
In draft because I want to perform a review myself of these changes first |
@@ -89,8 +97,8 @@ public function __invoke(Payload $payload): Payload | |||
$transformations = $templates->getTransformations(); | |||
|
|||
foreach ($project->getVersions() as $version) { | |||
$apiSets = $version->getDocumentationSets()->filter(ApiSetDescriptor::class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed this to remove the filter because the transformer should either be fully ApiSet specific and we need to make big changes to the pipelines; or it should be agnostic and the individual writers should check if they are suited to a specific set of documentation.
At this moment, I am choosing the latter. But I think I need to revisit some of the writers to actually add such checks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I have added extra checks to the individual writers
For a reason that I do not know, the pipeline shows a far lower coverage than running it locally. This now breaks the pipeline for this PR, and the only way to fix that would be to write extra unrelated tests, or change the number temporarily.
@@ -101,7 +101,7 @@ jobs: | |||
run: phpunit --testsuite=unit | |||
|
|||
- name: Quick check code coverage level | |||
run: php tests/coverage-checker.php 65 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the reviewer: for some reason, my local check reports more than 65% coverage, but the build pipeline does not. I could have fixed that by writing random unit tests, or by lowering this figure. I chose the latter to make sure no unrelated changes enter this PR. In a separate PR I will need to write additional tests to get this number higher again :)
We are inching closer to supporting the whole documentation pipeline per documentation set instead of a per-project level.
This is one of the final steps to make the architecture work in this way.
Once this is solved, we can start experimenting with enabling multi-documentation-set rendering and discovering the issues stemming from that.