-
-
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
Add a test to check that no old msgid or symbol are used #5839
Conversation
Pull Request Test Coverage Report for Build 1902891175
💛 - Coveralls |
@@ -15,6 +15,15 @@ Release date: TBA | |||
|
|||
Closes #352 | |||
|
|||
* ``using-f-string-in-unsupported-version`` and ``using-final-decorator-in-unsupported-version`` msgids |
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.
Hm, this is difficult. This is an important change, but this is technically breaking... Should it be in 3.0
?
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.
I think we need to fix this "fast" as we don't want to have a lot of content on the web with W1601 and W1602 meaning the new message. The breaking change is reasonable imo as it only breaks disable using msgid and not the symbol directly. (Also, the symbol should be preferred we even have a check for that so it's probably not affecting a lot of users ?)
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.
Yeah and I guess you could consider the previous change breaking. As in, we're reverting a previously unnoticed breaking change here.
Let's just go ahead with it.
The timeout for python 3.8 seems repeatable. Do we put change the value to 15 ? It could be because we're calculating the coverage for 3.8, but it seems we're doing - name: Upload coverage artifact
uses: actions/upload-artifact@v2.3.1
with:
name: coverage-${{ matrix.python-version }}
path: .coverage for all python version. And the other interpreter all take less than 4mn. |
Let me rebase on #5841 :) |
916e3df
to
06b2b19
Compare
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.
Small changes and questions. Rest looks good to go!
Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
Type of Changes
Description
This is a work in progress, I still need to make an exhaustive list of what was removed previously which is the hard part.I'm not sure if it's exhaustive but I can't think of any more msgid we deleted. Then we need to decide what we do when two name clash like forMessage id 'W1601' cannot have both 'apply-builtin' and 'using-f-string-in-unsupported-version' as symbolic name.
I think it's better to add a simple check and later on think about making the performance better for run time checking of the existence of a message in #5607.
Closes #5729