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

Fix tests for markdown 3, upgrade graphviz package #224

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ElementalWarrior
Copy link

  • Upgrade graphviz extension
  • assertEquals is a deprecated form of assertEqual
  • Test module does not exist, use src

renovate bot and others added 5 commits August 16, 2024 16:42
Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
markdown_inline_graphviz_extension version 1.1.3 will fix markdown > 3 support.
But the version only exists in the github: https://github.com/cesaremorel/markdown-inline-graphviz
It hasn't been published to pypi yet.

See: cesaremorel/markdown-inline-graphviz#13

For now, building from github may suffice
This code should probably be in an actual python package
@ElementalWarrior
Copy link
Author

Hi @awanlin. Any chance for a review?

@jonesbusy
Copy link
Contributor

Thanks.

This is related to #217

See also cesaremorel/markdown-inline-graphviz#13

Copy link
Collaborator

@awanlin awanlin left a comment

Choose a reason for hiding this comment

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

Hi @ElementalWarrior, can we trim this down, some of the changes are bine handled already in this PR #223. There also looked to be unrelated changes that crept in like the README.md

@ElementalWarrior
Copy link
Author

@awanlin I think thats just from merging the main branch into my feature branch (because a check was failing iirc).

So its github reporting things as changes that were already applied.

I can rebase it if you like. But It's effectively just re-listing already applied things.

@ElementalWarrior ElementalWarrior changed the base branch from renovate/markdown-3.x to main October 22, 2024 14:51
@ElementalWarrior
Copy link
Author

Ah, I think it was pointing at the wrong base branch to merge into.

Copy link
Collaborator

@awanlin awanlin left a comment

Choose a reason for hiding this comment

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

Hi @ElementalWarrior, I'd like to get in #223 first and then follow up with this one. Left a few comments in the mean time.


# The following are more akin to direct dependencies. Each line represents one
# or more features that are provided by `techdocs-core`, and thus are always
# pinned to an exact version. Bumps should be accompanied by release notes
# explaining what was added or fixed (or at least pointing to the underlying
# release notes of the bumped package).
mkdocs-material==9.5.42
markdown_inline_graphviz_extension==1.1.2
markdown_inline_graphviz_extension@git+https://github.com/cesaremorel/markdown-inline-graphviz.git@v1.1.3
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
markdown_inline_graphviz_extension@git+https://github.com/cesaremorel/markdown-inline-graphviz.git@v1.1.3
markdown_inline_graphviz_extension===1.1.3

Is there a reason we don't just do the above?

@@ -88,7 +88,7 @@ def test_theme_overrides_dirs(self):
def test_template_renders__multiline_value_as_valid_json(self):
self.techdocscore.on_config(self.mkdocs_yaml_config)
env = Environment(
loader=PackageLoader("test", self.techdocscore.tmp_dir_techdocs_theme.name),
loader=PackageLoader("src", self.techdocscore.tmp_dir_techdocs_theme.name),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this change? I don't see that being done in #223.

@deejay1 any thoughts on this?

@@ -98,7 +98,7 @@ def test_template_renders__multiline_value_as_valid_json(self):
}
rendered = template.render(config=config)
as_json = json.loads(rendered)
self.assertEquals(config, as_json)
self.assertEqual(config, as_json)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's revert this change as it will be done in #223

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.

3 participants