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

!!![TASK] Remove old rendering #27

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

jaapio
Copy link
Contributor

@jaapio jaapio commented Oct 14, 2024

This PR will remove the older sphinx rendering from this action.

Do not merge before the deadline to migrate to the new rendering

@linawolf linawolf marked this pull request as draft October 15, 2024 03:36
@linawolf
Copy link
Member

Setting this to draft so it does not accidently get rendered

@fe-hicking
Copy link

If I read this right this is missing the mandatory settings.cfg>guides.xml conversion step?

And we probably need to add some other migrations to alter Includes.txt dynamically? And some others that are "most common issues"?

@jaapio
Copy link
Contributor Author

jaapio commented Oct 15, 2024

Question is... Should we automate that? It might be better to just break as we might break the docs?

@linawolf
Copy link
Member

We promised to make an on-the-fly migration

@garvinhicking
Copy link
Contributor

From my experience, while the guides.xml automigration works quite flawless, usually there's always at least 1 or 2 directives that do not work out (shortcuts in Includes.txt, image inclusion). So probably the auto migration will never create a document that renders without errors (thus - will never get deployed).

But if there's a chance that 10 or 15% would work with an automigraiton, we should do that, it's better than 0% and letting them all fail.

@linawolf
Copy link
Member

These directives usually cause warnings not errors. So it would get rendered but a few links or directives would not work. So it would get rendered but not Flawlessly as if you had done a manual migration and ensured there are no warnings

@jaapio
Copy link
Contributor Author

jaapio commented Oct 16, 2024

Ok, I will re-add the step to detect the guides.xml and create one if it doesn't exist using the migration command.

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.

4 participants