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

Add new recursive argument to include directive #208

Merged
merged 10 commits into from
Jun 1, 2024

Conversation

carlocastoldi
Copy link
Contributor

Fixes: #207

Additionally, it also excludes Jupyter Notebooks from the substitution, in case users are using mkdocs-jupyter to import files with very long text as cell outputs

@carlocastoldi
Copy link
Contributor Author

somewhere, probably, i should write that if this is done to avoid very-long-text substitutions, than the user should use include-markdown.

If you think this can be handy only in this case scenario, i propose to make this directive only for include and swap the order of the substitutions.

Copy link
Owner

@mondeja mondeja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, would you mind to add tests and documentation? If you think it's too complicated, don't worry, I'll do it.

src/mkdocs_include_markdown_plugin/event.py Outdated Show resolved Hide resolved
src/mkdocs_include_markdown_plugin/event.py Outdated Show resolved Hide resolved
Copy link
Owner

@mondeja mondeja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This recursive argument must be added to the include-markdown directive as well.

README.md Outdated Show resolved Hide resolved
src/mkdocs_include_markdown_plugin/event.py Outdated Show resolved Hide resolved
@carlocastoldi
Copy link
Contributor Author

This recursive argument must be added to the include-markdown directive as well.

Do you see a scenario where markdown recursion might not be desirable, or is that just out of completeness?
I would argue that stopping recursion in markdown is never desirable while it adds confusion on the order of the substitutions now mattering when including very long lines

@mondeja
Copy link
Owner

mondeja commented May 29, 2024

I would argue that stopping recursion in markdown is never desirable while it adds confusion on the order of the substitutions now mattering when including very long lines

I don't think that it would add confusion, I don't understand where is the confusion here. Seems to me that it could be useful in certain scenarios, anyways not hurts to add it for completeness.

@carlocastoldi
Copy link
Contributor Author

I don't think that it would add confusion, I don't understand where is the confusion here. Seems to me that it could be useful in certain scenarios, anyways not hurts to add it for completeness.

This comment shows that it would add confusion, to me: #208 (comment)

But yeah, i'm okay with adding the possibility for markdown too. If you think it's really better. I just wanted to warn you on this

README.md Outdated Show resolved Hide resolved
@carlocastoldi carlocastoldi force-pushed the recursive-off branch 4 times, most recently from 3fd0faf to 409418b Compare May 30, 2024 12:55
@mondeja mondeja changed the title Add recursive directive Add recursive argument to include directive Jun 1, 2024
@mondeja mondeja changed the title Add recursive argument to include directive Add new recursive argument to include directive Jun 1, 2024
@mondeja mondeja merged commit 47f57fb into mondeja:master Jun 1, 2024
20 of 21 checks passed
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.

Add new argument recursive to include directive
2 participants