feat: Add initial restructured text docstirng parsing #71
Conversation
This implementation should be mostly complete. Not implemented: - references: Not planned at this time - examples: I'm not aware of a standard format for this Still needed: - more test: - Many of the error cases aren't currently covered - More full fixture test cases To be cleaned up: - I'm not confident I got the types right for all of the data that comes in with the context - ParseContext works, but it is a bit awkward with the rest of the project using the dict References: mkdocstrings#67
|
The PR still ended up being fairly big. Holding off on one of the main directives wouldn't have reduced the line count much. This seemed like a good stopping point to get something up for review. Subsequent PRs will be much smaller. |
|
Hello! Sorry for the delay, I've been quite busy 🙂 Thank you very much for this PR! I'm sure a lot of people will like it. |
|
As a heads up, I tried using this implementation against a real project and it was not getting the |
There was a problem hiding this comment.
I think I'm a bit too tired to read all that code closely 😅
But with a quick look I can tell the code is clean, you added comments to explain things, so LGTM.
Just a nitpick about the helpers in the test file.
About the signature of functions not being passed, maybe you could try to write a test that is actually loading an object, to see how it behaves from A to Z. I started thinking about the code structure, hopefully I'll find some time to improve it so it will be more straight-forward and easier to reason about.
| def assert_parameter_equal(actual: Parameter, expected: Parameter) -> None: | ||
| assert actual.name == expected.name | ||
| assert_annotated_obj_equal(actual, expected) | ||
| assert actual.kind == expected.kind | ||
| assert actual.default == expected.default | ||
|
|
||
|
|
||
| def assert_attribute_equal(actual: Attribute, expected: Attribute) -> None: | ||
| assert actual.name == expected.name | ||
| assert_annotated_obj_equal(actual, expected) | ||
|
|
||
|
|
||
| def assert_annotated_obj_equal(actual: AnnotatedObject, expected: AnnotatedObject) -> None: | ||
| assert actual.annotation == expected.annotation | ||
| assert actual.description == expected.description | ||
|
|
||
|
|
||
| def get_rst_object_documentation(dotted_fixture_subpath) -> Object: | ||
| return Loader(docstring_style="restructured-text").get_object_documentation( | ||
| f"tests.fixtures.parsing.restructured_text.{dotted_fixture_subpath}" | ||
| ) |
There was a problem hiding this comment.
Could you move these helpers at the top of the file? I find it less confusing, and we'll be able to add test at the end without having to think about pushing the helpers further down.
There was a problem hiding this comment.
Sure. I generally do it with the helper functions at the end, but it doesn't matter much. Another option would be to implement __eq__() on the section detail objects, This would allow for assert sections[1].value == [EXPECTED_PARAM]. I opted not to do that to keep my PR more self-contained without making changes to the other structures.
|
Oh, it would also be really great if you could update the README 🙂 |
Add test cases to increase coverage
|
I was able to track down there error in my code related to the context. (A helper function in the google test suite passes the signature and type in as part of the context. However, the main source code only ever passes in I also added more test cases complete coverage. |
|
Wanted to check back in. Is there anything else you would like me to add to this PR before it can be merged? |
|
Hello! I've been lacking time and energy... but I'll try to give a final review tomorrow 🙂 |
This implementation should be mostly complete.
Not implemented:
Still needed:
To be cleaned up:
References: #67