-
Notifications
You must be signed in to change notification settings - Fork 0
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
Sourcery refactored develop branch #1
base: develop
Are you sure you want to change the base?
Conversation
version = tuple([int(n) for n in value[0].strip().split('.')]) | ||
version = tuple(int(n) for n in value[0].strip().split('.')) |
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.
Function repository
refactored with the following changes:
- Replace unneeded comprehension with generator (
comprehension-to-generator
)
cmd = [sevenzip, 'x', archive, '-o' + location, '-y'] | ||
cmd = [sevenzip, 'x', archive, f'-o{location}', '-y'] |
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.
Function unpack
refactored with the following changes:
- Use f-string instead of string concatenation (
use-fstring-for-concatenation
)
matches = re_content.match(content) | ||
if matches: | ||
if matches := re_content.match(content): |
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.
Function download
refactored with the following changes:
- Use named expression to simplify assignment and conditional [×2] (
use-named-expression
) - Swap if/else to remove empty if body (
remove-pass-body
) - Lift code into else after jump in control flow (
reintroduce-else
) - Swap if/else branches (
swap-if-else-branches
) - Use f-string instead of string concatenation (
use-fstring-for-concatenation
)
if revision == None: | ||
if revision is None: |
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.
Function root
refactored with the following changes:
- Use x is None rather than x == None (
none-compare
) - Use f-string instead of string concatenation (
use-fstring-for-concatenation
)
description = 'Comparing %s to %s' % (test_baseline, test_contender) | ||
description = f'Comparing {test_baseline} to {test_contender}' |
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.
Function main
refactored with the following changes:
- Replace interpolated string formatting with f-string [×7] (
replace-interpolation-with-fstring
)
if s.startswith('/'): | ||
return " " # note: a space and not an empty string | ||
else: | ||
return s | ||
return " " if s.startswith('/') else s | ||
|
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.
Function strip_comments
refactored with the following changes:
- Replace if statement with if expression (
assign-if-exp
)
This removes the following comments ( why? ):
# note: a space and not an empty string
for m in matches: | ||
if match.start() > m.start() and \ | ||
match.end() < m.end(): | ||
return True | ||
return False | ||
return any( | ||
match.start() > m.start() and match.end() < m.end() for m in matches | ||
) |
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.
Function _is_within
refactored with the following changes:
- Use any() instead of for loop (
use-any
)
match = pattern.search(self.content, index) | ||
if match: | ||
if match := pattern.search(self.content, index): |
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.
Function TranslationUnit._search_content
refactored with the following changes:
- Use named expression to simplify assignment and conditional (
use-named-expression
)
@@ -41,6 +41,7 @@ | |||
same line, but it is far from perfect (in either direction). | |||
""" | |||
|
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.
Lines 560-569
refactored with the following changes:
- Replace interpolated string formatting with f-string [×6] (
replace-interpolation-with-fstring
)
if sys.version_info < (3,): | ||
return codecs.unicode_escape_decode(x)[0] | ||
else: | ||
return x | ||
return codecs.unicode_escape_decode(x)[0] if sys.version_info < (3,) else x |
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.
Function unicode_escape_decode
refactored with the following changes:
- Replace if statement with if expression (
assign-if-exp
)
_hpp_headers = set(['h', 'hh', 'hpp', 'hxx', 'h++', 'cuh']) | ||
_hpp_headers = {'h', 'hh', 'hpp', 'hxx', 'h++', 'cuh'} |
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.
Lines 681-681
refactored with the following changes:
- Unwrap a constant iterable constructor (
unwrap-iterable-construction
)
return GetHeaderExtensions().union(set(['c', 'cc', 'cpp', 'cxx', 'c++', 'cu'])) | ||
return GetHeaderExtensions().union({'c', 'cc', 'cpp', 'cxx', 'c++', 'cu'}) |
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.
Function GetAllExtensions
refactored with the following changes:
- Unwrap a constant iterable constructor (
unwrap-iterable-construction
)
matched = Search(r'\bNOLINT(NEXTLINE)?\b(\([^)]+\))?', raw_line) | ||
if matched: | ||
if matched.group(1): | ||
suppressed_line = linenum + 1 | ||
else: | ||
suppressed_line = linenum | ||
category = matched.group(2) | ||
if category in (None, '(*)'): # => "suppress all" | ||
_error_suppressions.setdefault(None, set()).add(suppressed_line) | ||
else: | ||
if category.startswith('(') and category.endswith(')'): | ||
category = category[1:-1] | ||
if category in _ERROR_CATEGORIES: | ||
_error_suppressions.setdefault(category, set()).add(suppressed_line) | ||
elif category not in _LEGACY_ERROR_CATEGORIES: | ||
error(filename, linenum, 'readability/nolint', 5, | ||
'Unknown NOLINT error category: %s' % category) | ||
if not (matched := Search(r'\bNOLINT(NEXTLINE)?\b(\([^)]+\))?', raw_line)): | ||
return | ||
suppressed_line = linenum + 1 if matched.group(1) else linenum | ||
category = matched.group(2) | ||
if category in (None, '(*)'): # => "suppress all" | ||
_error_suppressions.setdefault(None, set()).add(suppressed_line) | ||
elif category.startswith('(') and category.endswith(')'): | ||
category = category[1:-1] | ||
if category in _ERROR_CATEGORIES: | ||
_error_suppressions.setdefault(category, set()).add(suppressed_line) | ||
elif category not in _LEGACY_ERROR_CATEGORIES: | ||
error( | ||
filename, | ||
linenum, | ||
'readability/nolint', | ||
5, | ||
f'Unknown NOLINT error category: {category}', | ||
) |
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.
Function ParseNolintSuppressions
refactored with the following changes:
- Use named expression to simplify assignment and conditional (
use-named-expression
) - Add guard clause (
last-if-guard
) - Replace if statement with if expression (
assign-if-exp
) - Merge else clause's nested if statement into elif (
merge-else-if-into-elif
) - Replace interpolated string formatting with f-string (
replace-interpolation-with-fstring
)
if (self._last_header > header_path and | ||
Match(r'^\s*#\s*include\b', clean_lines.elided[linenum - 1])): | ||
return False | ||
return True | ||
return self._last_header <= header_path or not Match( | ||
r'^\s*#\s*include\b', clean_lines.elided[linenum - 1]) |
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.
Function _IncludeState.IsInAlphabeticalOrder
refactored with the following changes:
- Lift code into else after jump in control flow (
reintroduce-else
) - Replace if statement with if expression (
assign-if-exp
) - Simplify boolean if expression (
boolean-if-exp-identity
) - Remove unnecessary casts to int, str, float or bool (
remove-unnecessary-cast
)
error_message = ('Found %s after %s' % | ||
(self._TYPE_NAMES[header_type], | ||
self._SECTION_NAMES[self._section])) | ||
error_message = f'Found {self._TYPE_NAMES[header_type]} after {self._SECTION_NAMES[self._section]}' |
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.
Function _IncludeState.CheckNextIncludeOrder
refactored with the following changes:
- Replace interpolated string formatting with f-string (
replace-interpolation-with-fstring
)
error(filename, 0, 'build/header_guard', 5, | ||
'No #ifndef header guard found, suggested CPP variable is: %s' % | ||
cppvar) | ||
error( | ||
filename, | ||
0, | ||
'build/header_guard', | ||
5, | ||
f'No #ifndef header guard found, suggested CPP variable is: {cppvar}', | ||
) | ||
return | ||
|
||
# The guard should be PATH_FILE_H_, but we also allow PATH_FILE_H__ | ||
# for backward compatibility. | ||
if ifndef != cppvar: | ||
error_level = 0 | ||
if ifndef != cppvar + '_': | ||
error_level = 5 | ||
|
||
error_level = 5 if ifndef != f'{cppvar}_' else 0 | ||
ParseNolintSuppressions(filename, raw_lines[ifndef_linenum], ifndef_linenum, | ||
error) | ||
error(filename, ifndef_linenum, 'build/header_guard', error_level, | ||
'#ifndef header guard has wrong style, please use: %s' % cppvar) | ||
error( | ||
filename, | ||
ifndef_linenum, | ||
'build/header_guard', | ||
error_level, | ||
f'#ifndef header guard has wrong style, please use: {cppvar}', | ||
) | ||
|
||
# Check for "//" comments on endif line. | ||
ParseNolintSuppressions(filename, raw_lines[endif_linenum], endif_linenum, | ||
error) | ||
match = Match(r'#endif\s*//\s*' + cppvar + r'(_)?\b', endif) | ||
if match: | ||
if match := Match(r'#endif\s*//\s*' + cppvar + r'(_)?\b', endif): | ||
if match.group(1) == '_': | ||
# Issue low severity warning for deprecated double trailing underscore | ||
error(filename, endif_linenum, 'build/header_guard', 0, | ||
'#endif line should be "#endif // %s"' % cppvar) | ||
error( | ||
filename, | ||
endif_linenum, | ||
'build/header_guard', | ||
0, | ||
f'#endif line should be "#endif // {cppvar}"', | ||
) |
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.
Function CheckForHeaderGuard
refactored with the following changes:
- Use named expression to simplify assignment and conditional [×2] (
use-named-expression
) - Swap positions of nested conditionals [×14] (
swap-nested-ifs
) - Move setting of default value for variable into
else
branch (introduce-default-else
) - Replace interpolated string formatting with f-string [×5] (
replace-interpolation-with-fstring
) - Use f-string instead of string concatenation (
use-fstring-for-concatenation
) - Replace if statement with if expression (
assign-if-exp
)
basefilename = filename[0:len(filename) - len(fileinfo.Extension())] | ||
headerfile = basefilename + '.' + ext | ||
basefilename = filename[:len(filename) - len(fileinfo.Extension())] | ||
headerfile = f'{basefilename}.{ext}' |
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.
Function CheckHeaderFileIncluded
refactored with the following changes:
- Replace a[0:x] with a[:x] and a[x:len(a)] with a[x:] (
remove-redundant-slice-index
) - Use f-string instead of string concatenation [×2] (
use-fstring-for-concatenation
) - Replace interpolated string formatting with f-string (
replace-interpolation-with-fstring
)
error(filename, linenum, 'runtime/threadsafe_fn', 2, | ||
'Consider using ' + multithread_safe_func + | ||
'...) instead of ' + single_thread_func + | ||
'...) for improved thread safety.') | ||
error( | ||
filename, | ||
linenum, | ||
'runtime/threadsafe_fn', | ||
2, | ||
f'Consider using {multithread_safe_func}...) instead of {single_thread_func}...) for improved thread safety.', | ||
) |
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.
Function CheckPosixThreading
refactored with the following changes:
- Use f-string instead of string concatenation [×4] (
use-fstring-for-concatenation
)
if linenum > 0 and Search(r'\\$', clean_lines[linenum - 1]): | ||
return True | ||
|
||
return False | ||
return bool(linenum > 0 and Search(r'\\$', clean_lines[linenum - 1])) |
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.
Function IsMacroDefinition
refactored with the following changes:
- Lift code into else after jump in control flow (
reintroduce-else
) - Replace if statement with if expression (
assign-if-exp
) - Simplify boolean if expression (
boolean-if-exp-identity
)
match = Search( | ||
if match := Search( | ||
r'\b(DISALLOW_COPY_AND_ASSIGN|DISALLOW_IMPLICIT_CONSTRUCTORS)\(' + | ||
self.name + r'\)', | ||
clean_lines.elided[i]) | ||
if match: | ||
clean_lines.elided[i], | ||
): | ||
if seen_last_thing_in_class: | ||
error(filename, i, 'readability/constructors', 3, | ||
match.group(1) + ' should be the last thing in the class') | ||
error( | ||
filename, | ||
i, | ||
'readability/constructors', | ||
3, | ||
f'{match.group(1)} should be the last thing in the class', | ||
) |
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.
Function _ClassInfo.CheckEnd
refactored with the following changes:
- Use named expression to simplify assignment and conditional (
use-named-expression
) - Replace if statement with if expression (
assign-if-exp
) - Replace interpolated string formatting with f-string (
replace-interpolation-with-fstring
) - Use f-string instead of string concatenation [×3] (
use-fstring-for-concatenation
)
error( | ||
filename, | ||
linenum, | ||
'readability/namespace', | ||
5, | ||
f'Namespace should be terminated with "// namespace {self.name}"', | ||
) | ||
elif not Match(r'^\s*};*\s*(//|/\*).*\bnamespace[\*/\.\\\s]*$', line): | ||
# If "// namespace anonymous" or "// anonymous namespace (more text)", | ||
# mention "// anonymous namespace" as an acceptable form | ||
if Match(r'^\s*}.*\b(namespace anonymous|anonymous namespace)\b', line): | ||
error(filename, linenum, 'readability/namespace', 5, | ||
'Namespace should be terminated with "// namespace %s"' % | ||
self.name) | ||
else: | ||
# Anonymous namespace | ||
if not Match(r'^\s*};*\s*(//|/\*).*\bnamespace[\*/\.\\\s]*$', line): | ||
# If "// namespace anonymous" or "// anonymous namespace (more text)", | ||
# mention "// anonymous namespace" as an acceptable form | ||
if Match(r'^\s*}.*\b(namespace anonymous|anonymous namespace)\b', line): | ||
error(filename, linenum, 'readability/namespace', 5, | ||
'Anonymous namespace should be terminated with "// namespace"' | ||
' or "// anonymous namespace"') | ||
else: | ||
error(filename, linenum, 'readability/namespace', 5, | ||
'Anonymous namespace should be terminated with "// namespace"') | ||
'Anonymous namespace should be terminated with "// namespace"' | ||
' or "// anonymous namespace"') | ||
else: | ||
error(filename, linenum, 'readability/namespace', 5, | ||
'Anonymous namespace should be terminated with "// namespace"') |
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.
Function _NamespaceInfo.CheckEnd
refactored with the following changes:
- Merge else clause's nested if statement into elif (
merge-else-if-into-elif
) - Replace interpolated string formatting with f-string (
replace-interpolation-with-fstring
)
This removes the following comments ( why? ):
# Anonymous namespace
else: | ||
# TODO(unknown): unexpected #else, issue warning? | ||
pass |
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.
Function NestingState.UpdatePreprocessor
refactored with the following changes:
- Remove redundant pass statement [×2] (
remove-redundant-pass
)
This removes the following comments ( why? ):
# TODO(unknown): unexpected #endif, issue warning?
# TODO(unknown): unexpected #else, issue warning?
if self.stack: | ||
self.previous_stack_top = self.stack[-1] | ||
else: | ||
self.previous_stack_top = None | ||
|
||
self.previous_stack_top = self.stack[-1] if self.stack else None |
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.
Function NestingState.Update
refactored with the following changes:
- Replace if statement with if expression [×2] (
assign-if-exp
) - Use named expression to simplify assignment and conditional (
use-named-expression
) - Merge else clause's nested if statement into elif (
merge-else-if-into-elif
) - Replace multiple comparisons of same variable with
in
operator (merge-comparisons
) - Move setting of default value for variable into
else
branch (introduce-default-else
) - Replace interpolated string formatting with f-string (
replace-interpolation-with-fstring
) - Use f-string instead of string concatenation [×2] (
use-fstring-for-concatenation
)
This removes the following comments ( why? ):
# token == '}'
# Perform end of block checks and pop the stack.
error(filename, obj.starting_linenum, 'build/class', 5, | ||
'Failed to find complete declaration of class %s' % | ||
obj.name) | ||
error( | ||
filename, | ||
obj.starting_linenum, | ||
'build/class', | ||
5, | ||
f'Failed to find complete declaration of class {obj.name}', | ||
) | ||
elif isinstance(obj, _NamespaceInfo): | ||
error(filename, obj.starting_linenum, 'build/namespaces', 5, | ||
'Failed to find complete declaration of namespace %s' % | ||
obj.name) | ||
error( | ||
filename, | ||
obj.starting_linenum, | ||
'build/namespaces', | ||
5, | ||
f'Failed to find complete declaration of namespace {obj.name}', | ||
) |
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.
Function NestingState.CheckCompletedBlocks
refactored with the following changes:
- Replace interpolated string formatting with f-string [×2] (
replace-interpolation-with-fstring
)
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.
PR Type: Refactoring
Summary of PR: This PR introduces a series of refactoring changes made by the Sourcery tool to the develop
branch. The changes include the use of f-strings for string formatting, simplification of conditional expressions, and the use of more concise Python syntax such as set literals and the walrus operator.
General PR suggestions
- Review the use of the walrus operator for assignment within conditionals and loops to ensure readability is maintained, especially for those who may not be familiar with this newer Python syntax.
- Verify the logic changes, particularly where the original logic has been inverted or significantly altered, to ensure that the intended behavior is preserved.
- Consider adding comments to explain complex one-liners or less common syntax that may not be immediately clear to future maintainers of the code.
- Evaluate the compatibility of the refactored code with the minimum supported Python version, as some changes may utilize features not available in older Python versions.
- Ensure that all refactoring changes are thoroughly tested to prevent regressions or unintended side effects.
Your trial expires on December 18, 2023. Please email tim@sourcery.ai to continue using Sourcery ✨
return codecs.unicode_escape_decode(x)[0] | ||
else: | ||
return x | ||
return codecs.unicode_escape_decode(x)[0] if sys.version_info < (3,) else x |
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.
suggestion (llm): Consider simplifying the conditional expression for readability.
return codecs.unicode_escape_decode(x)[0] if sys.version_info < (3,) else x | |
return x if sys.version_info >= (3,) else codecs.unicode_escape_decode(x)[0] |
elif category not in _LEGACY_ERROR_CATEGORIES: | ||
error(filename, linenum, 'readability/nolint', 5, | ||
'Unknown NOLINT error category: %s' % category) | ||
if not (matched := Search(r'\bNOLINT(NEXTLINE)?\b(\([^)]+\))?', raw_line)): |
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.
suggestion (llm): Using the walrus operator (:=) inside an if statement without parentheses can reduce readability. Consider using a separate assignment statement.
if not (matched := Search(r'\bNOLINT(NEXTLINE)?\b(\([^)]+\))?', raw_line)): | |
matched = Search(r'\bNOLINT(NEXTLINE)?\b(\([^)]+\))?', raw_line) | |
+ if not matched: |
return False | ||
return True | ||
return self._last_header <= header_path or not Match( | ||
r'^\s*#\s*include\b', clean_lines.elided[linenum - 1]) |
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.
issue (llm): The logic for checking alphabetical order of headers seems to have been inverted. This change may cause incorrect behavior when determining if headers are in the correct order.
elided = tail[second_quote + 1:] | ||
else: | ||
# Unmatched double quote, don't bother processing the rest | ||
# of the line since this is probably a multiline string. | ||
collapsed += elided | ||
break | ||
elif Search(r'\b(?:0[bBxX]?|[1-9])[0-9a-fA-F]*$', head): |
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.
question (llm): Is the intention here to match numeric literals for further processing? It would be helpful to add a comment explaining the purpose of this new conditional branch.
m = label_re.match(l) | ||
if m: | ||
found.add('.L%s' % m.group(1)) | ||
if m := label_re.match(l): |
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.
suggestion (llm): While the walrus operator is a nice addition to Python 3.8, it may reduce readability for those unfamiliar with the syntax. Consider if compatibility with earlier Python versions is a concern.
if m := label_re.match(l): | |
m = label_re.match(l) | |
+ if m: |
add_line = True | ||
break | ||
if add_line: | ||
if add_line := next( |
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.
issue (llm): The use of assignment expression (walrus operator) with generator expressions and the 'all' function can be confusing. Consider refactoring for clarity.
@@ -1275,8 +1269,7 @@ | |||
if self.lines_in_function > trigger: | |||
error_level = int(math.log(self.lines_in_function / base_trigger, 2)) | |||
# 50 => 0, 100 => 1, 200 => 2, 400 => 3, 800 => 4, 1600 => 5, ... | |||
if error_level > 5: | |||
error_level = 5 | |||
error_level = min(error_level, 5) |
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.
praise (llm): Good use of the min function to simplify the capping of error_level to 5.
Branch
develop
refactored by Sourcery.If you're happy with these changes, merge this Pull Request using the Squash and merge strategy.
See our documentation here.
Run Sourcery locally
Reduce the feedback loop during development by using the Sourcery editor plugin:
Review changes via command line
To manually merge these changes, make sure you're on the
develop
branch, then run:Help us improve this pull request!