Show module docstrings in the explore UI#346
Conversation
|
Hi, thanks for your contribution! It's an interesting idea. Sorry I haven't got around to reviewing this yet, but I will do over the next few days. |
seddonym
left a comment
seddonym
left a comment
There was a problem hiding this comment.
Thanks for your submission!
I like this idea, but I think the visual design needs a bit of work before it's ready to go in.
In particular:
- Line breaks should be respected.
- Not so keen on the italics running across the top of the page. Could we try it in the top of the right sidebar, in non-italic text? Maybe it should be collapsible in case the text is long.
- I think it should possibly be clearer where the text has come from - e.g. an unobtrusive label so people know it's from the module docstring.
- Non-blocking, but the tooltip hovering doesn't work well for long module docstrings. Maybe we should just display the first line when hovering?
Here's an example with django.templates which might be a good reference point.
Might take a bit of back and forth to get this one right, if you're not up for that, I completely understand!
|
Thanks for the detailed feedback! I've addressed all the points:
Let me know if you'd like any further adjustments! |
seddonym
left a comment
seddonym
left a comment
There was a problem hiding this comment.
Sorry for the delay in reviewing, this is looking pretty good!
Have left some comments, happy to discuss them / hear your ideas.
| spec = importlib.util.find_spec(module) | ||
| if spec is None or spec.origin is None: | ||
| return None | ||
| if not spec.origin.endswith("__init__.py"): |
There was a problem hiding this comment.
What's the reason for this check?
| if not spec.origin.endswith("__init__.py"): | ||
| return None | ||
| try: | ||
| source = Path(spec.origin).read_text(encoding="utf-8") |
There was a problem hiding this comment.
Python files aren't necessarily utf-8 encoded.
In Grimp (the library that Import Linter uses for static analysis) we read files using tokenize.open.
https://github.com/python-grimp/grimp/blob/main/src/grimp/adaptors/filesystem.py#L35
There's a test for handling these kinds of files here. We should probably have similar coverage for this here, do you agree?
| tree = ast.parse(source) | ||
| return ast.get_docstring(tree) | ||
| except Exception: | ||
| return None |
There was a problem hiding this comment.
Could we log a warning with the exception message here? Would be helpful for debugging.
| def test_returns_child_descriptions(self, client): | ||
| response = client.get("/api/graph/testpackage") | ||
| data = response.json() | ||
| assert "child_descriptions" in data |
There was a problem hiding this comment.
Could we assert on the entire value of child_descriptions? It'll be clearer what we expect there, and we get a better diff if the test fails.
|
|
||
| ## latest | ||
|
|
||
| * Show module docstring below the breadcrumb bar when exploring a package. |
There was a problem hiding this comment.
This comment is out of date, it's in the sidebar.
|
|
||
| ### Module descriptions | ||
|
|
||
| When a package has a docstring in its `__init__.py`, a brief description is shown below |
There was a problem hiding this comment.
These docs need updating to reflect the new UI.
|
|
||
| ### Module descriptions | ||
|
|
||
| When a package has a docstring in its `__init__.py`, a brief description is shown below |
There was a problem hiding this comment.
It is a bit difficult to read longer module descriptions in the UI.
What do you think about making it possible to resize the sidebar? Not sure how difficult that would be.
2 participants
When using
import-linter explore, the UI now surfaces module docstrings in two places:__init__.pyis shown in italic, giving immediate context about the module being explored.
docstring (if available). For package nodes, the tooltip also shows the name of the
module you'll navigate to on click.
Docstrings are extracted statically via
ast.parse()+ast.get_docstring(), so nomodule code is executed during rendering.
Checklist
just check.docs/release_notes.md.docs/authors.md(in alphabetical order).