Finish updating CTS to pass pylint and pyflake #3820
+287
−253
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nrwahl2
reviewed
Feb 8, 2025
|
This PR is already too big. I'm going to split it into one that's everything before dealing with make_test_group, and another that's that and everything after it. |
Just like everything else in cts/, we need to tell pylint to not worry about import order, file names, or comparisons at the top of the file.
A bunch of these strings are giant anyway and run well past the 80 character mark. Several were still continued onto a second line even though none of them are the longest description. So just condense them into single lines.
This is just the standard technique of inverting conditionals in order to return early, and then unindenting all the code after the conditional.
This is just referenced in one spot, there's no need to have it be a variable.
Fixing this is a larger problem than is addressable in this patch set - it requires rethinking how cts-scheduler is put together, which I don't want to work on at the moment. In the interest of running pylint and pyflake as part of the CI process without failing every build, I'm disabling this warning for now.
Yes, we know there's a lot of duplicate code in some of our regression test tools. In the interest of running pylint and pyflake as part of the CI process without failing every build, I'm disabling this warning for now.
This is a list of generated python scripts that can have pylint and pyflake run against them. It exists because we were working on these scripts one at a time, and I didn't want to overwhelm the results with tons of errors from scripts we hadn't gotten around to yet. The problem with this is that sometimes, we forget to add scripts to the list. That's the case with cts-cli, which is why it still needs to be converted to using f-strings. When we convert cts-schemas to python, this can all go away and we can use test_SCRIPTS instead.
The string substitutions I was doing with s.format() through the
apply_substitutions function won't work once we move to f-strings. I
don't think that function is doing anything special, however, so we can
just get rid one substitution it's performing at a time.
This removes all uses of {shadow} that don't occur in a string with some
other substitution.
Just like the previous commit, there's still places where we can't completely switch over to f-strings for performing this substition, so it has to stay in the apply_substitutions function for now. However, there are several places we no longer need to call that function because the only thing we were substituting was cts_cli_home. Additionally, test_home wasn't being substituted anywhere so it can go.
clumens
force-pushed
the
final-pylint
branch
from
ff3bfd3 to
4e64a39
Compare
|
Updated to address review comments, including a new patch at the end to deal with extra whitespace. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I originally also wanted to include pylint and pyflake as part of "make check", but then I'd have to BuildRequires them and I don't think we want to do that (I haven't checked, but those tools may not be in all supported OSes).