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

Change writers to be performed on a per-documentation-set basis #3496

Merged
merged 7 commits into from
Mar 27, 2023

Conversation

mvriel
Copy link
Member

@mvriel mvriel commented Mar 25, 2023

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.

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.
@mvriel
Copy link
Member Author

mvriel commented Mar 25, 2023

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);
Copy link
Member Author

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

Copy link
Member Author

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

@mvriel mvriel marked this pull request as ready for review March 25, 2023 09:35
mvriel added 2 commits March 26, 2023 10:04
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
Copy link
Member Author

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 :)

@mvriel mvriel marked this pull request as draft March 26, 2023 11:17
@mvriel mvriel marked this pull request as ready for review March 26, 2023 11:20
@jaapio jaapio merged commit 68ead7c into master Mar 27, 2023
@mvriel mvriel deleted the transform-per-documentation-set branch March 27, 2023 19:47
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