search

backstage / mkdocs-techdocs-core

The core MkDocs plugin used by Backstage's TechDocs as a wrapper around multiple MkDocs plugins and Python Markdown extensions
Apache License 2.0
87 stars 64 forks source link

Loosen the pinned version requirement for Markdown #27

Closed n2ygk closed 3 years ago

n2ygk commented 3 years ago

Fixes: #26

Since requirements.txt is directly used by setup.py this constrains this package too much when combined with other packages that require a newer version of Markdown.

OrkoHunter commented 3 years ago

For reference, why the version was pinned -> https://github.com/backstage/backstage/pull/3139 (In Spotify, a later version of Markdown broke all the graphviz images). So, this should be carefully tested. But +1 to upgrades. :)

n2ygk commented 3 years ago

Can you provide a test graphviz file?

On Thu, Jun 24, 2021 at 5:54 AM Himanshu Mishra @.***> wrote:

For reference, why the version was pinned -> backstage/backstage#3139 https://github.com/backstage/backstage/pull/3139 (In Spotify, a later version of Markdown broke all the graphviz images). So, this should be carefully tested. But +1 to upgrades. :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/backstage/mkdocs-techdocs-core/pull/27#issuecomment-867503442, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBHS56ENI7MQCCW64MSJQLTUL6ELANCNFSM47GSDR7A .

n2ygk commented 3 years ago

Can you provide a test graphviz file?

@OrkoHunter ^

OrkoHunter commented 3 years ago

Hey @n2ygk ! Sorry, for my delayed response. I only meant my original comment as a reference for @backstage/techdocs-core to be careful about, when upgrading the internal techdocs container.

I think this PR is useful to the community and should be merged.

n2ygk commented 3 years ago

Hey @n2ygk ! Sorry, for my delayed response. I only meant my original comment as a reference for @backstage/techdocs-core to be careful about, when upgrading the internal techdocs container.

I think this PR is useful to the community and should be merged.

Sure but it would be easy enough to test against something that was known to break it in the past....

iamEAP commented 3 years ago

Thanks for the patience @n2ygk! Merged and released as 0.1.0.

OrkoHunter commented 3 years ago

Just a note that this was reverted in https://github.com/backstage/mkdocs-techdocs-core/pull/37

@backstage/techdocs-core Should we open an issue to keep track of this issue? I think it's quite likely that in future folks would want to do this change again. It'd be nice to have an issue around.

n2ygk commented 3 years ago

Just a note that this was reverted in #37

@backstage/techdocs-core Should we open an issue to keep track of this issue? I think it's quite likely that in future folks would want to do this change again. It'd be nice to have an issue around.

I did ask for an example that breaks but didn't get a response so I was unable to add a test.

emmaindal commented 3 years ago

Hey @OrkoHunter thanks for pointing this out, I was actually looking for exactly this discussion as I had some faint memory of this being discussed before, but couldn't find it. So yes I think it would be good with an issue tracking this. Do any of you want to open one specify the need for not having a pinned version? @n2ygk @OrkoHunter And then I can try to find a example of a breaking diagram to attach to the issue (which is the reason to why its pinned again)

OrkoHunter commented 3 years ago

specify the need for not having a pinned version

I think the reason from @n2ygk is justified - "Since requirements.txt is directly used by setup.py this constrains this package too much when combined with other packages that require a newer version of Markdown."