Skip to content

Conversation

@ucodery
Copy link
Contributor

@ucodery ucodery commented May 23, 2022

First off I think this library is great and I use it to test all my markdown readmes. However, I also write a lot of bash examples in these readmes and would like a way to also test those. I hope you will consider this increased scope to the tool so it can grow to cover all code examples in markdown.

@koaning
Copy link
Owner

koaning commented May 24, 2022

I'm happy to hear that this library has been useful!

While I am interested in this feature, my recommendation is to first ask if such a feature is appreciated on GitHub issues. It's fine now, but for another time and another project, it's best to check in with the maintainer first.

I'll try to give a quick review now.

@koaning
Copy link
Owner

koaning commented May 24, 2022

I noticed the tests didn't run on this PR, but that's due to a bad config on my end. Made a PR to fix it #9.

Could you pull main such that the tests run here as well?

Copy link
Owner

@koaning koaning left a comment

Choose a reason for hiding this comment

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

Hey, this was a nice PR! I really like the registry that you've added there 👍 it'll allow folks to even implement their own checkers/linters.

I left some minor comments for now. I just realised that the tests weren't running, and I'd like to give it another glance once those look green.

Comment on lines +119 to +131
This will print the python version to the terminal

```bash
python --version
```

This will print the exact same version string

```python
import sys

print(f"Python {sys.version_info.major}.{sys.version_info.minor}.{sys.version_info.micro}")
```
Copy link
Owner

@koaning koaning May 26, 2022

Choose a reason for hiding this comment

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

I couldn't help but notice you using an extra tab here, presumably to display the ticks. I figured I'd share a trick, because there is an alternative syntax for moments like this. No need to change anything here, but I typically share this trick because many folks seem unaware.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was actually just copying the md examples from elsewhere in the README. But no, I did not know that other way of including backtics; it will certainly be useful in the future.

I do find it interesting that I am finding out about this markdown formatting while working on mktestdocs. I believe the current mktestdocs would not execute these valid quadruple or pentuple, etc, code blocks. Perhaps that would be a valuable addition in the future?

Copy link
Owner

@koaning koaning May 26, 2022

Choose a reason for hiding this comment

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

Whoa. This is indeed getting meta 😅 I'm the maintainer of mktestdocs and I did not even realise that the extra-ticks-markdown-formatting is related to the project.

I think I prefer to see it as an advanced use case that might be best to skip for now. I'd be interested in understanding the use-case a bit better before actually thinking about the feature. It should be easy for folks to build their own implementation in the meantime.

Copy link
Owner

@koaning koaning left a comment

Choose a reason for hiding this comment

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

It's looking great! I just added one comment on invalid JSON. It might be good to add a test case for it. The readme file right now shows the declaration of a test using a custom JSON checker, but not an actual run.

@ucodery
Copy link
Contributor Author

ucodery commented May 26, 2022

There was supposed to already be a test case for the json, which is that when test_mktestdocs runs the README, the example json parser is actually executed. It was an oversight that the example was not actually run.

Copy link
Owner

@koaning koaning left a comment

Choose a reason for hiding this comment

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

LGTM.

@koaning
Copy link
Owner

koaning commented May 26, 2022

It's my turn to cook tonight, so I'll merge now and I'll whip up the next release later tonight.

But I'm super excited to merge this one in. It'll make the project so much more flexible. Thanks for the PR! 😄

@koaning koaning merged commit 6822ffa into koaning:main May 26, 2022
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.

2 participants