Fix E501 line length violations in tests/integrations/ and tests/platforms/ - #2979
Fix E501 line length violations in tests/integrations/ and tests/platforms/#2979phyodev wants to merge 14 commits into
Conversation
|
Originally posted by @phyodev in #2383
|
freakboy3742
left a comment
There was a problem hiding this comment.
There's no need to tell us how you ran the test suite - you just need to run it. The tests also need to pass - and CI is currently showing problems.
A key detail in this refactor I that we need to retain readability. It's not enough to just split strings into multiple parts - the final result needs to be clear to read as well.
Consider the following:
"this is a long string"
"that runs across lines\n"
"but how many lines are there?\n"
That visually looks like 3 lines, but is actually 2. That's potentially very confusing to debug - so we need to find ways to structure the code so that ambiguity doesn't exist.
That might involve moving the test output to a standalone file, as is done in a lot of the Android/ADB integration tests.
| "Intel x86_64 Atom System Image | " | ||
| "system-images/android-31/default/x86_64\n" |
There was a problem hiding this comment.
These two lines could be combined into one to maintain a consistent 2 fields per line, which would make the table readable even when wrapped.
There was a problem hiding this comment.
On second thoughts, this should probably be exempted from the line length limit, as discussed below.
| "------- | -------\n" | ||
| " system-images;android-31;default;x86_64 | 5 | " | ||
| "Intel x86_64 Atom System Image | " | ||
| "system-images/android-31/default/x86_64\n" |
There was a problem hiding this comment.
See comment on similar code above.
|
Thanks for your feedback. Let me review and fix my changes to align with your suggestions |
Thanks for your feedback and let me fix it |
| ), | ||
| ( | ||
| "\x1b[32mD/libEGL : loaded /vendor/lib64/egl/libEGL_emulation.so\x1b[0m", | ||
| ("\x1b[32mD/libEGL : loaded /vendor/lib64/egl/libEGL_emulation.so\x1b[0m"), |
There was a problem hiding this comment.
Unnecessary parentheses (multiple instances in this file, and other files; I won't bother marking any more).
| ( | ||
| "test_case (tests.foobar.test_other.TestOtherMethods)", | ||
| True, | ||
| ), |
There was a problem hiding this comment.
Unnecessary change (multiple instances in this file).
| / ( | ||
| "base_path/build/first-app/android/gradle/" | ||
| "app/build/python/pip/debug/common" | ||
| ) |
There was a problem hiding this comment.
| / ( | |
| "base_path/build/first-app/android/gradle/" | |
| "app/build/python/pip/debug/common" | |
| ) | |
| / "base_path/build/first-app/android/gradle" | |
| / "app/build/python/pip/debug/common" |
There was a problem hiding this comment.
There are multiple instances of this in other files; I won't bother marking any more of them.
| # zip file can include a “__MACOSX” folder for | ||
| # each document that contains information about | ||
| # the file useful for Finder and will not be in the unzipped set of files |
| # As a result of mocking ditto, the zip archive won't *actually* be created; | ||
| # and as a result of mocking os, it won't *actually* be deleted either - but we can | ||
| # verify that it *would* have been deleted. ditto will also be called when finalizing, | ||
| # verify that it *would* have been deleted. | ||
| # ditto will also be called when finalizing, |
| # Package the app without code signing. | ||
| # Use the base command's interface to ensure the full | ||
| # cleanup process is tested. |
There was a problem hiding this comment.
Uneven line length: could probably be 2 lines if properly wrapped.
| == "Ad-hoc identity. The resulting package will run but cannot be re-distributed." | ||
| adhoc.name == "Ad-hoc identity. The resulting package will run " | ||
| "but cannot be re-distributed." | ||
| ) |
There was a problem hiding this comment.
adhoc_name should be outside the parentheses, in which case the string might not even need to be split.
| ( | ||
| " 0 DTDeviceKitBase " | ||
| "0x00000001288ff298 DTDKCreateNSErrorFromAMDErrorCode + 300" | ||
| ), |
There was a problem hiding this comment.
For testing purposes, this could be reduced to only 2 stack trace entries.
| import { hooks } from "https://pyscript.net/""" | ||
| """releases/2024.11.1/core.js"; |
There was a problem hiding this comment.
In this case, and all others involving HTML and CSS blocks, I think skipping the check with a noqa comment would be better.
|
Please don't mark conversations as resolved: let the person who started the conversation do that. Automatic resolution from accepting a suggestion is OK. |
For multi-line strings, especially if they're structured as a table or something, maybe the best solution would be to keep it as a triple-quoted string with |
cb9f792 to
cabebb8
Compare
@mhsmith Hey! I updated my changes according to your feedbacks. But I am not still merge main to my branch yet because there is some CI failures. But I believe my codes are aligned accordingly. If further more needed feel free to tell me |
This PR is to fix E501 errors under tests/integrations and tests/platforms
E501ignored Ruff rule #2383PR Checklist:
Assisted-by: Gemini Plus (as Chatbot)