Add support for more complicated documentation examples #8287
Conversation
DanielNoord
added
Documentation π
Skip news π
| @@ -0,0 +1,9 @@ | |||
| from .bad2 import count_to_two # [cyclic-import] | |||
There was a problem hiding this comment.
Can we take into account the fact that if we want to do multiple examples properly displayed we're going to want to have bad.py, bad2.py, bad3.py (...) too ? Maybe the display can change if there's no good2.py, (...) matching the bad2.py (...) ? Edit: Did not notice it's in a directory, I think it's compatible.
doc/test_messages_documentation.py
Outdated
| if (message_dir / "bad.py").exists(): | ||
| suite.append( | ||
| (message_dir.stem, message_dir / "bad.py"), | ||
| ) | ||
| elif (message_dir / "bad").is_dir(): |
There was a problem hiding this comment.
Haa, this make sense, we can have directories too for multiple example. (bad / bad2/...)
There was a problem hiding this comment.
I think in that case it makes more sense to just use one file
There was a problem hiding this comment.
You could also do bad.py, neutral.py, lawful.py, chaotic.py and good.py
pylint/checkers/imports.py
Outdated
| if imported_module is None: | ||
|
|
||
| # If we can't find the import we still try and build the import graph with our | ||
| # best guess: the relative name of the import. | ||
| imported_module_name = imported_module.name if imported_module else node.modname | ||
| if imported_module_name is None: | ||
| return | ||
|
|
||
| for name, _ in node.names: | ||
| if name != "*": | ||
| self._add_imported_module(node, f"{imported_module.name}.{name}") | ||
| self._add_imported_module(node, f"{imported_module_name}.{name}") | ||
| else: | ||
| self._add_imported_module(node, imported_module.name) | ||
| self._add_imported_module(node, imported_module_name) |
There was a problem hiding this comment.
Seems like something that could be it's own PR.
There was a problem hiding this comment.
Yeah and it is breaking tests... Without it this test is very flaky so I'm not sure what to do
DanielNoord
force-pushed
the
docs-messages
branch
from
f46f18b
to
7227c07
Compare
|
@Pierre-Sassoulas I kept in the I think this PR also closes #7143 right? |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #8287 +/- ##
==========================================
- Coverage 95.46% 95.46% -0.01%
==========================================
Files 177 177
Lines 18704 18702 -2
==========================================
- Hits 17856 17854 -2
Misses 848 848
|
Yes, amazing ! |
doc/exts/pylint_messages.py
Outdated
| ) | ||
| elif (data_path / "bad").exists(): | ||
| bad_one = f"""\ | ||
| File one: |
There was a problem hiding this comment.
Instead of File one / File two, we should have the actual name of the file (bad.py::. This would also simplify the handling code that can become generic and handle as many file as we want. We can also name it however we want (it's the directory prefix that indicate badness / goodness).
There was a problem hiding this comment.
I like this suggestion! Will (hopefully) work on this tomorrow!
doc/exts/pylint_messages.py
Outdated
| if bad_py_path.exists(): | ||
| bad_code = _get_titled_rst( | ||
| title="Problematic code", text=_get_python_code_as_rst(bad_py_path) | ||
| ) | ||
| elif (data_path / "bad").exists(): | ||
| bad_files: list[str] = [] | ||
| for file in (data_path / "bad").iterdir(): | ||
| if file.suffix == ".py": | ||
| bad_files.append( | ||
| f"""\ | ||
| ``{file.name}``: | ||
|
|
||
| .. literalinclude:: /{file.relative_to(Path.cwd())} | ||
| :language: python | ||
|
|
||
| """ | ||
| ) | ||
| bad_code = _get_titled_rst(title="Problematic code", text="".join(bad_files)) | ||
| else: | ||
| bad_code = "" |
There was a problem hiding this comment.
We should create a generic function for this as we also need to handle good/ directories with multiple file imo. I can do it, it's a lot easier with pycharm than vscode.
There was a problem hiding this comment.
Yeah I didn't want to add code that we don't actively use right now, but go ahead and add it! Whatever makes it easier to complete this process the quickest!
doc/exts/pylint_messages.py
Outdated
| ) | ||
| elif (data_path / "bad").exists(): | ||
| bad_files: list[str] = [] | ||
| for file in (data_path / "bad").iterdir(): |
There was a problem hiding this comment.
We need to sort here, so bad.py is before bad2.py
| for file in (data_path / "bad").iterdir(): | |
| for file in sorted((data_path / "bad").iterdir()): |
There was a problem hiding this comment.
Turn out it's not that easy to sort Path π took me embarrassingly long to understand
|
I have commits almost ready that clean up this PR. Hopefully can finish this tomorrow. |
DanielNoord
force-pushed
the
docs-messages
branch
from
99ef71b
to
d2d50d9
Compare
|
Not clearing that reporter after every file shows some underlying issues that we never seen. Nice to fix these as well I think π |
Yeah, like I said; the If you know of another message where you want multiple files and that isn't flakey I can add it as well. Otherwise I think we should just merge this. I did test this locally with multiple files and checked that the tests still worked (which is how I found the issues with the reporter). |
|
Maybe too many lines ? It's a little insulting to readers though. Otherwise, duplicated code ? I can add this and test locally myself :) |
Duplicated code would be a good one! Perhaps make a PR that has this branch as target branch? |
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Pierre-Sassoulas
force-pushed
the
docs-messages
branch
from
9453e4c
to
bbfd103
Compare
|
Both https://pylint--8287.org.readthedocs.build/en/8287/user_guide/messages/warning/while-used.html and https://pylint--8287.org.readthedocs.build/en/8287/user_guide/messages/refactor/duplicate-code.html are definite improvements! Indeed happy we have this now! |
btw - is the |
|
I think it might make sense indeed! |
In pylint-dev#7162, "we decreed" that the `bad_code` goes first, then the `good_code`. Fixup current changes, to revert "back to standards". Additionally, tackle the comment here pylint-dev#8287 (comment) "Message emitted" is technically still code (`%-formatting`); let us make it stand out properly. Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
In #7162, "we decreed" that the `bad_code` goes first, then the `good_code`. Fixup current changes, to revert "back to standards". Additionally, tackle the comment here #8287 (comment) "Message emitted" is technically still code (`%-formatting`); let us make it stand out properly. Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
Type of Changes
Description
Refs #7897
Closes #7143