-
-
Notifications
You must be signed in to change notification settings - Fork 9
Fix duplicate docs generation into backend #204
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
Conversation
Something remaining to be discussed here is what to do with the |
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.
LGTM. Let's see if we can get an answer regarding your question before merging. Thank you!
I did a little digging around. It was part of the initial commit, but without comment for its existence. I assume that Érico had a plan for it at the time, and using |
I would remove the empty docs folder in the backend scaffold. The new documentation starter scaffold takes its place. |
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.
Tested. Success!
@davisagli @ericof should we change the CI to allow it to run from forks? There are two jobs that are waiting for a status to be reported. Here's what we did in Volto: https://github.com/plone/volto/blob/main/.github/workflows/acceptance.yml#L2-L5 But I don't know how to merge it with our https://github.com/plone/cookieplone-templates/blob/main/.github/workflows/main.yml#L4-L6 Also can we merge this PR? |
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.
Wouldn't it be a bit more straightforward to make the project template explicitly set initialize_documentation to false in the context that it uses when it calls the backend_addon template as a subtemplate? https://github.com/plone/cookieplone-templates/blob/main/templates/projects/monorepo/hooks/post_gen_project.py#L64
@stevepiercy I don't like the solution we used for Volto because it always shows lots of skipped checks. What we've done elsewhere is set the workflow to trigger on a push event for the main branch or a pull_request event that targets the main branch. |
I had to look up the term "context" in Cookiecutter docs, but its definition doesn't exist. This is the file I tried to examine: https://cookiecutter.readthedocs.io/en/stable/cookiecutter.html. It appears there is a context dict, a context template, and a context project. Anyway, @davisagli, is your suggestion the same as mine in the original issue, IOW, prevent creating the |
@davisagli got link to example? I can try to replicate its usage for cookieplone-templates in a separate PR. |
I'm assuming that would mean adding the |
Update: I have tried a new approach, which follows @davisagli advice. Also some formatting fixes. |
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.
Minor tweaks for consistency
Checked out, generated project, ran uv run pytest tests/templates/add-ons/documentation_starter/test_cutter.py |
Co-authored-by: Steve Piercy <[email protected]>
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.
LGTM. Let's get @davisagli to review the changes.
@stevepiercy done in #212 |
@davisagli could I get a review from you on @ujsquared's changes per your feedback to him? Thank you! |
@stevepiercy Yes, I will. Had to take a break to make dinner |
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.
This is making it a bit more complicated than it needs to be. It was easier to show what I wanted to do differently rather than explain how to do it, so I made a new PR: #218
Let's get this closed and merge #218 instead. |
Closing in favor of #218 |
Fixes #202
I think I missed these two lines while restructuring. Already present in
frontend_addon