-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix duplicate names of Pylint Messages
in docs
#5744
Fix duplicate names of Pylint Messages
in docs
#5744
Conversation
Pull Request Test Coverage Report for Build 1808554416
π - Coveralls |
@Pierre-Sassoulas What are the titles of the pages in the side bar for you? For me the seconde title doesn't seem to be updated.. |
Same here. It's strange, although it would work as it is it look like there's no page containing "Overview" in the generated code : https://pylint--5744.org.readthedocs.build/en/5744/search.html?q=Overview |
Hmm, I'll have a look at this later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a bug in readthedoc for this MR ? We can merge anyway as "pylint messages" makes sense for Overview of all Pylint messages
?
That would be strange.. I'd say let's wait to find the cause of the issue before merging. |
Fixed. Stupid mistake... The page was actually auto-generated, so changing the title in the file does nothing π |
Should we had warning as comment at the beginning of the file ? Or maybe check that the file is up to date. |
I don't think we should check for it being up to date, that would mean every change to a message would add changes to the file. Which would inflate commit sizes unnecessarily. I'll add a comment though! |
Could you add a release stage in tbump to make sure it's up to date at release time then ? (See https://github.com/PyCQA/pylint/blob/main/tbump.toml#L31) |
Do we want it to be? (I have no experience with |
I think it's better if it's not too far from the current state of the code. Another positive things is that the diff is a nice summary of what message appeared between two releases.
I made the change myself let me know what you think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! Thanks for your help!
doc/exts/pylint_messages.py
Outdated
@@ -122,7 +122,10 @@ def _write_messages_list_page( | |||
stream.write( | |||
f""".. _messages-list: | |||
|
|||
{get_rst_title("Pylint Messages", "=")} | |||
.. NOTE: This file is auto-generated. Make any changes to the associated | |||
.. docs extension in '{__name__}.py'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smart!
Build is failing though :( Do you see what's wrong? |
Hurgh, sorry it was working locally. It sounds like the problem is |
@Pierre-Sassoulas Have you had the time to look at this? We should probably try and merge this soon. I can also take a look? |
I have no idea right now, and I have a busy week. What I tried :
What I thought about doing next:
Ideas welcomed :) |
β¦duplicate-names
Damn, it was this 'simple' in the end. Thank you for the debug work @DanielNoord, I'm under the water right now and appreciate that. |
Same, this was an easy fix I could do in between jobs π |
Type of Changes
Description
Pretty stupid to have missed this. Should be merged asap to help SEO.
Closes #5740.