Repository navigation
Added CodeDescription to LSPReporter and changed docs links to use HTTPS - #1411
Conversation
Coverage Report for CI Build 39088830895Coverage increased (+0.002%) to 91.587%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
david-yz-liu
left a comment
There was a problem hiding this comment.
Thanks @angelayzheng! In response to your questions:
- The PythonTA documentation should include information for all pylint errors that we enable by default, so if there are missing ones we should resolve that in a future PR.
- Even though this is an addition to the reporter, because we don't have documentation on each reporter don't worry about updating the documentation for this change.
| severity=_lsp_severity(msg.category), | ||
| code=msg.msg_id, | ||
| code_description=types.CodeDescription( | ||
| href=f"{python_ta.HELP_URL}#{msg.msg_id.lower()}" |
There was a problem hiding this comment.
This is good, but shouldn't duplicate logic. Instead, please create a new python_ta.util function to return the URL and use it both here and for the main doc function.
At the same time, please also modify the HELP_URL to use https instead of http; add an extra CHANGELOG entry for this under "Enhancements".
There was a problem hiding this comment.
Thank you for your feedback! I've created a new helper function, get_documentation_url, with the shared behaviour. I've also updated the HELP_URL to use https instead of http, along with the corresponding tests, snapshots, and CHANGELOG.
I did delete two outdated snapshot files that are no longer used (I found them when I was searching for the old http HELP_URL): no_watch_html_server_snapshot.html and watch_html_server_snapshot.html. These snapshots' tests were moved in commit 4b2dac0 but weren't deleted at the time.
I believe the repository likely contains additional outdated snapshots, but I assume that this would be part of a different refactoring PR. Please let me know if you think these two snapshots should not be deleted as part of my current PR.
…ng/pyta into lsp-reporter-code-description
david-yz-liu
left a comment
There was a problem hiding this comment.
It looks like there are two snapshot files that have been deleted, which you should regenerate:
packages/python-ta/tests/test_reporters/snapshots/test_html_server/test_open_html_in_browser_no_watch/no_watch_html_server_snapshot.htmlpackages/python-ta/tests/test_reporters/snapshots/test_html_server/test_open_html_in_browser_watch/watch_html_server_snapshot.html
| @@ -0,0 +1,20 @@ | |||
| """Tests for PythonTA documentation URL generation.""" | |||
There was a problem hiding this comment.
This is good but to keep files organized, please put this under tests/test_util/test_documentation.py.
Hi @david-yz-liu, I explained why I deleted these two files in my reply to your previous comment. Could you please review that and let me know if these can be kept as deleted? Thanks! |
|
@angelayzheng thanks for the explanation; you can remove these files in a separate PR, but it shouldn't be part of this one. |
|
@david-yz-liu thanks! I've moved |
david-yz-liu
left a comment
There was a problem hiding this comment.
Nice work, @angelayzheng!
Proposed Changes
Added a new
CodeDescriptionfield toLSPReporter, which contains a URL to the corresponding documentation section for each diagnostic message. It currently uses the same link as thepython_ta.docfunction.In VS Code, the
CodeDescriptionis displayed as a clickable hyperlink in the error code, as shown in the screenshots below.Additionally, changed all documentation URLs and accompanying tests to use HTTPS instead of the previous HTTP.
Screenshots of your changes (if applicable)
Without
CodeDescription:With
CodeDescription:Type of Change
(Write an
Xor a brief description next to the type or types that best describe your changes.)Checklist
(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the
[ ]into a[x]in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)Before opening your pull request:
After opening your pull request:
Questions and Comments
Not all of the error codes that are caught by PythonTA have a corresponding documentation section on the PythonTA website. For example,
C0114: Missing module docstringcurrently links to this URL: https://www.cs.toronto.edu/~david/pyta/checkers/index.html#c0114 (which brings you to the top of the page since the heading does not exist), whereas the correct Pylint documentation is here: https://pylint.readthedocs.io/en/latest/user_guide/messages/convention/missing-module-docstring.htmlWhat is the desired behaviour when there is no PythonTA documentation section? I think it would be confusing to link to a nonexistent section, but this is following the current behaviour of the
python_ta.docfunction. I also understand if implementing a check for whether the section exists is outside the scope of this feature.Additionally, which documentation pages (if any) should I update as part of this feature?