search

backstage / mkdocs-monorepo-plugin

✚ Build multiple documentation folders in a single Mkdocs. Designed for large codebases.
https://backstage.github.io/mkdocs-monorepo-plugin/
Apache License 2.0
314 stars 75 forks source link

fix: don't run on_serve if on_config was skipped #69

Closed richardmcsong closed 2 years ago

richardmcsong commented 2 years ago

I didn't mean to steal @gferon 's thunder, but I saw that #62 was somewhat stale. I decided to review the code a little bit and I discovered that the whole of on_serve should be skipped entirely if on_config didn't run, since there would therefore be no extra files to livereload to watch.

I do have some outstanding questions -- do we want to augment our github actions workflows to also run these unit tests?

Also, I'm going to have to apologize if I'm not following some style guides; it's been a while since I've written python!

Architecture

Since this plugin is not functional without nav, on_config detects whether this needs to be run as a first step, and returns immediately if it is not found.

However, currently on_serve fails with a TypeError if on_config returns immediately, since it tries to server.watch(None). Therefore, we should also skip on_serve if on_config never ran in the first place.

Other changes

ottosichert commented 2 years ago

Hey @richardmcsong, thanks a lot for your contribution, especially for the tests!

The code itself looks all right and does solve the issue.

For the tests, we currently have a setup using Bats for integration tests, so I'm trying to figure out what the best way is to integrate your test case which seems more like a unit test.

One solution I could think of is moving the tests files to __tests__/unit and invoking unittest from __tests__/test-*.sh.

What do you think about this approach?

richardmcsong commented 2 years ago

That's a wonderful approach! Unfortunately I've been absolutely smacked with work, so I haven't had time to try it out. I'll give it a try by the end of the week -- just dropping by to say I haven't forgotten about this!

zhammer commented 2 years ago

@richardmcsong was about to push a similar PR. happy to see you're working on this as it's become quite frustrating on our end.

richardmcsong commented 2 years ago

@ottosichert On trying the approach, I'm starting to disagree with moving the tests into the __tests__ folder. Primarily, python3 -m unittest discover only imports tests that are valid python identifiers; with __tests__ in its name, it ignores the test folder unless you set the search start directory to __tests__ itself. Considering that workaround, though, unit tests should stay closer to the code than integration tests do. I propose leaving the tests in its original place, but running them still from the __tests__/test-*.sh.

richardmcsong commented 2 years ago

@ottosichert r4r again!

camilaibs commented 2 years ago

@ottosichert r4r again!

Hi @richardmcsong, thanks again, I'm taking over @ottosichert's review this week and I'll go back to you today 🙂

satrox28 commented 2 years ago

Hi @ottosichert can you please help us to get the PR merged.

suuus commented 2 years ago

Hey @richardmcsong, congrats on your first merged PR 🎉 Nice work 👍 Please let me know if we can be helpful in any way.