refactor ui #6125
Closed
refactor ui #6125
Conversation
This file contains hidden or 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
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- core.py has grown to over a thousand lines—consider splitting it into smaller, focused modules (e.g. input, formatting, option-parsing) to improve readability and maintainability.
- There are many
# type: ignoreannotations sprinkled throughout—refining the type signatures and adding missing return annotations could allow you to remove these suppressions and tighten up type checking. - The public API surface in
ui/__init__.pyis quite large; reducing the number of names exported via__all__or grouping related functions behind submodules could simplify the user-facing interface.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- core.py has grown to over a thousand lines—consider splitting it into smaller, focused modules (e.g. input, formatting, option-parsing) to improve readability and maintainability.
- There are many `# type: ignore` annotations sprinkled throughout—refining the type signatures and adding missing return annotations could allow you to remove these suppressions and tighten up type checking.
- The public API surface in `ui/__init__.py` is quite large; reducing the number of names exported via `__all__` or grouping related functions behind submodules could simplify the user-facing interface.
## Individual Comments
### Comment 1
<location> `beets/ui/colors.py:158` </location>
<code_context>
+ return ANSI_CODE_REGEX.sub("", colored_text)
+
+
+def color_split(colored_text: str, index: int):
+ length: int = 0
+ pre_split: str = ""
</code_context>
<issue_to_address>
**suggestion:** Function color_split lacks a return type annotation.
Please add a return type annotation to color_split to enhance code clarity and support static analysis tools.
```suggestion
def color_split(colored_text: str, index: int) -> tuple[str, str]:
```
</issue_to_address>
### Comment 2
<location> `beets/ui/colors.py:173-176` </location>
<code_context>
def color_split(colored_text: str, index: int):
length: int = 0
pre_split: str = ""
post_split: str = ""
found_color_code: str | None = None
found_split: bool = False
part: str
for part in ANSI_CODE_REGEX.split(colored_text) or ():
# Count how many real letters we have passed
length += color_len(part)
if found_split:
post_split += part
else:
if ANSI_CODE_REGEX.match(part):
# This is a color code
if part == RESET_COLOR:
found_color_code = None
else:
found_color_code = part
pre_split += part
else:
if index < length:
# Found part with our split in.
split_index: int = index - (length - color_len(part))
found_split = True
if found_color_code:
pre_split += f"{part[:split_index]}{RESET_COLOR}"
post_split += f"{found_color_code}{part[split_index:]}"
else:
pre_split += part[:split_index]
post_split += part[split_index:]
else:
# Not found, add this part to the pre split
pre_split += part
return pre_split, post_split
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace if statement with if expression ([`assign-if-exp`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/assign-if-exp/))
```suggestion
found_color_code = None if part == RESET_COLOR else part
```
</issue_to_address>
### Comment 3
<location> `beets/ui/colors.py:256-259` </location>
<code_context>
def colordiff(a: object, b: object) -> tuple[str, str]:
"""Colorize differences between two values if color is enabled.
(Like _colordiff but conditional.)
"""
if config["ui"]["color"]:
return _colordiff(a, b)
else:
return str(a), str(b)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace if statement with if expression ([`assign-if-exp`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/assign-if-exp/))
```suggestion
return _colordiff(a, b) if config["ui"]["color"] else (str(a), str(b))
```
</issue_to_address>
### Comment 4
<location> `beets/ui/commands.py:204-206` </location>
<code_context>
def func(self, lib, opts, args):
if args:
cmdname = args[0]
helpcommand = self.root_parser._subcommand_for_name(cmdname)
if not helpcommand:
raise UserError(f"unknown command '{cmdname}'")
helpcommand.print_help()
else:
self.root_parser.print_help()
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
- Lift code into else after jump in control flow ([`reintroduce-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/reintroduce-else/))
- Swap if/else branches ([`swap-if-else-branches`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/swap-if-else-branches/))
</issue_to_address>
### Comment 5
<location> `beets/ui/commands.py:433` </location>
<code_context>
def show_match_details(self):
"""Print out the details of the match, including changes in album name
and artist name.
"""
# Artist.
artist_l, artist_r = self.cur_artist or "", self.match.info.artist
if artist_r == VARIOUS_ARTISTS:
# Hide artists for VA releases.
artist_l, artist_r = "", ""
if artist_l != artist_r:
artist_l, artist_r = colordiff(artist_l, artist_r)
left = {
"prefix": f"{self.changed_prefix} Artist: ",
"contents": artist_l,
"suffix": "",
}
right = {"prefix": "", "contents": artist_r, "suffix": ""}
self.print_layout(self.indent_detail, left, right)
else:
print_(f"{self.indent_detail}*", "Artist:", artist_r)
if self.cur_album:
# Album
album_l, album_r = self.cur_album or "", self.match.info.album
if (
self.cur_album != self.match.info.album
and self.match.info.album != VARIOUS_ARTISTS
):
album_l, album_r = colordiff(album_l, album_r)
left = {
"prefix": f"{self.changed_prefix} Album: ",
"contents": album_l,
"suffix": "",
}
right = {"prefix": "", "contents": album_r, "suffix": ""}
self.print_layout(self.indent_detail, left, right)
else:
print_(f"{self.indent_detail}*", "Album:", album_r)
elif self.cur_title:
# Title - for singletons
title_l, title_r = self.cur_title or "", self.match.info.title
if self.cur_title != self.match.info.title:
title_l, title_r = colordiff(title_l, title_r)
left = {
"prefix": f"{self.changed_prefix} Title: ",
"contents": title_l,
"suffix": "",
}
right = {"prefix": "", "contents": title_r, "suffix": ""}
self.print_layout(self.indent_detail, left, right)
else:
print_(f"{self.indent_detail}*", "Title:", title_r)
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Extract duplicate code into method ([`extract-duplicate-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-duplicate-method/))
- Combine two compares on same value into a chained compare ([`chain-compares`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/chain-compares/))
</issue_to_address>
### Comment 6
<location> `beets/ui/commands.py:1332-1338` </location>
<code_context>
def import_files(lib, paths: list[bytes], query):
"""Import the files in the given list of paths or matching the
query.
"""
# Check parameter consistency.
if config["import"]["quiet"] and config["import"]["timid"]:
raise UserError("can't be both quiet and timid")
# Open the log.
if config["import"]["log"].get() is not None:
logpath = syspath(config["import"]["log"].as_filename())
try:
loghandler = logging.FileHandler(logpath, encoding="utf-8")
except OSError:
raise UserError(
"Could not open log file for writing:"
f" {displayable_path(logpath)}"
)
else:
loghandler = None
# Never ask for input in quiet mode.
if config["import"]["resume"].get() == "ask" and config["import"]["quiet"]:
config["import"]["resume"] = False
session = TerminalImportSession(lib, loghandler, paths, query)
session.run()
# Emit event.
plugins.send("import", lib=lib, paths=paths)
</code_context>
<issue_to_address>
**issue (code-quality):** Explicitly raise from a previous error ([`raise-from-previous-error`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/raise-from-previous-error/))
</issue_to_address>
### Comment 7
<location> `beets/ui/commands.py:2063` </location>
<code_context>
def modify_func(lib, opts, args):
query, mods, dels = modify_parse_args(args)
if not mods and not dels:
raise UserError("no modifications specified")
modify_items(
lib,
mods,
dels,
query,
should_write(opts.write),
should_move(opts.move),
opts.album,
not opts.yes,
opts.inherit,
)
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Lift code into else after jump in control flow ([`reintroduce-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/reintroduce-else/))
- Swap if/else branches ([`swap-if-else-branches`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/swap-if-else-branches/))
- Split conditional into multiple branches ([`split-or-ifs`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/split-or-ifs/))
- Merge duplicate blocks in conditional ([`merge-duplicate-blocks`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/merge-duplicate-blocks/))
- Remove redundant conditional ([`remove-redundant-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-redundant-if/))
</issue_to_address>
### Comment 8
<location> `beets/ui/commands.py:2391` </location>
<code_context>
def config_edit():
"""Open a program to edit the user configuration.
An empty config file is created if no existing config file exists.
"""
path = config.user_config_path()
editor = util.editor_command()
try:
if not os.path.isfile(path):
open(path, "w+").close()
util.interactive_open([path], editor)
except OSError as exc:
message = f"Could not edit configuration: {exc}"
if not editor:
message += (
". Please set the VISUAL (or EDITOR) environment variable"
)
raise UserError(message)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Explicitly raise from a previous error ([`raise-from-previous-error`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/raise-from-previous-error/))
```suggestion
raise UserError(message) from exc
```
</issue_to_address>
### Comment 9
<location> `beets/ui/core.py:88-90` </location>
<code_context>
def _stream_encoding(stream: TextIO | Any, default: str = "utf-8") -> str:
"""A helper for `_in_encoding` and `_out_encoding`: get the stream's
preferred encoding, using a configured override or a default
fallback if neither is not specified.
"""
# Configured override?
encoding: str = config["terminal_encoding"].get()
if encoding:
return encoding
# For testing: When sys.stdout or sys.stdin is a StringIO under the
# test harness, it doesn't have an `encoding` attribute. Just use
# UTF-8.
if not hasattr(stream, "encoding"):
return default
# Python's guessed output stream encoding, or UTF-8 as a fallback
# (e.g., when piped to a file).
return stream.encoding or default
</code_context>
<issue_to_address>
**suggestion (code-quality):** Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
```suggestion
if encoding := config["terminal_encoding"].get():
```
</issue_to_address>
### Comment 10
<location> `beets/ui/core.py:203-204` </location>
<code_context>
def input_(prompt: str | None = None) -> str:
"""Like `input`, but decodes the result to a Unicode string.
Raises a UserError if stdin is not available. The prompt is sent to
stdout rather than stderr. A printed between the prompt and the
input cursor.
"""
# raw_input incorrectly sends prompts to stderr, not stdout, so we
# use print_() explicitly to display prompts.
# https://bugs.python.org/issue1927
if prompt:
print_(prompt, end=" ")
try:
resp: str = input()
except EOFError:
raise UserError("stdin stream ended while input required")
return resp
</code_context>
<issue_to_address>
**suggestion (code-quality):** Explicitly raise from a previous error ([`raise-from-previous-error`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/raise-from-previous-error/))
```suggestion
except EOFError as e:
raise UserError("stdin stream ended while input required") from e
```
</issue_to_address>
### Comment 11
<location> `beets/ui/core.py:304-308` </location>
<code_context>
def input_options(
options: Iterable[str],
require: bool = False,
prompt: str | None = None,
fallback_prompt: str | None = None,
numrange: tuple[int, int] | None = None,
default: int | str | None = None,
max_width: int = 72,
) -> int | str:
"""Prompts a user for input. The sequence of `options` defines the
choices the user has. A single-letter shortcut is inferred for each
option; the user's choice is returned as that single, lower-case
letter. The options should be provided as lower-case strings unless
a particular shortcut is desired; in that case, only that letter
should be capitalized.
By default, the first option is the default. `default` can be provided to
override this. If `require` is provided, then there is no default. The
prompt and fallback prompt are also inferred but can be overridden.
If numrange is provided, it is a pair of `(high, low)` (both ints)
indicating that, in addition to `options`, the user may enter an
integer in that inclusive range.
`max_width` specifies the maximum number of columns in the
automatically generated prompt string.
"""
# Assign single letters to each option. Also capitalize the options
# to indicate the letter.
letters: dict[str, str] = {}
display_letters: list[str] = []
capitalized: list[str] = []
first: bool = True
option: str
for option in options:
# Is a letter already capitalized?
letter: str
found_letter: str
for letter in option:
if letter.isalpha() and letter.upper() == letter:
found_letter = letter
break
else:
# Infer a letter.
for letter in option:
if not letter.isalpha():
continue # Don't use punctuation.
if letter not in letters:
found_letter = letter
break
else:
raise ValueError("no unambiguous lettering found")
letters[found_letter.lower()] = option
index: int = option.index(found_letter)
# Mark the option's shortcut letter for display.
show_letter: str
is_default: bool
if not require and (
(default is None and not numrange and first)
or (
isinstance(default, str)
and found_letter.lower() == default.lower()
)
):
# The first option is the default; mark it.
show_letter = f"[{found_letter.upper()}]"
is_default = True
else:
show_letter = found_letter.upper()
is_default = False
# Colorize the letter shortcut.
show_letter = colorize(
"action_default" if is_default else "action", show_letter
)
# Insert the highlighted letter back into the word.
descr_color: ColorName = (
"action_default" if is_default else "action_description"
)
capitalized.append(
colorize(descr_color, option[:index])
+ show_letter
+ colorize(descr_color, option[index + 1 :])
)
display_letters.append(found_letter.upper())
first = False
# The default is just the first option if unspecified.
if require:
default = None
elif default is None:
if numrange:
default = numrange[0]
else:
default = display_letters[0].lower()
# Make a prompt if one is not provided.
if not prompt:
prompt_parts: list[str] = []
prompt_part_lengths: list[int] = []
if numrange:
if isinstance(default, int):
default_name: str = str(default)
default_name = colorize("action_default", default_name)
tmpl: str = "# selection (default {})"
prompt_parts.append(tmpl.format(default_name))
prompt_part_lengths.append(len(tmpl) - 2 + len(str(default)))
else:
prompt_parts.append("# selection")
prompt_part_lengths.append(len(prompt_parts[-1]))
prompt_parts += capitalized
prompt_part_lengths += [len(s) for s in options]
# Wrap the query text.
# Start prompt with U+279C: Heavy Round-Tipped Rightwards Arrow
prompt = colorize("action", "\u279c ")
line_length: int = 0
i: int
part: str
length: int
for i, (part, length) in enumerate(
zip(prompt_parts, prompt_part_lengths)
):
# Add punctuation.
if i == len(prompt_parts) - 1:
part += colorize("action_description", "?")
else:
part += colorize("action_description", ",")
length += 1
# Choose either the current line or the beginning of the next.
if line_length + length + 1 > max_width:
prompt += "\n"
line_length = 0
if line_length != 0:
# Not the beginning of the line; need a space.
part = f" {part}"
length += 1
prompt += part
line_length += length
# Make a fallback prompt too. This is displayed if the user enters
# something that is not recognized.
if not fallback_prompt:
fallback_prompt = "Enter one of "
if numrange:
fallback_prompt += "{}-{}, ".format(*numrange)
fallback_prompt += f"{', '.join(display_letters)}:"
resp: int | str | None = input_(prompt)
while True:
if isinstance(resp, str):
resp = resp.strip().lower()
# Try default option.
if default is not None and not resp:
resp = default
# Try an integer input if available.
if numrange:
try:
resp = int(resp) # type: ignore[arg-type]
except ValueError:
pass
else:
low: int
high: int
low, high = numrange
if low <= resp <= high:
return resp
else:
resp = None
# Try a normal letter input.
if isinstance(resp, str):
resp = resp[0]
if resp in letters:
return resp
# Prompt for new input.
resp = input_(fallback_prompt)
</code_context>
<issue_to_address>
**suggestion (code-quality):** We've found these issues:
- Replace if statement with if expression ([`assign-if-exp`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/assign-if-exp/))
- Low code quality found in input\_options - 4% ([`low-code-quality`](https://docs.sourcery.ai/Reference/Default-Rules/comments/low-code-quality/))
```suggestion
default = numrange[0] if numrange else display_letters[0].lower()
```
<br/><details><summary>Explanation</summary>
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines.
- Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.</details>
</issue_to_address>
### Comment 12
<location> `beets/ui/core.py:477-482` </location>
<code_context>
def get_replacements() -> list[tuple[re.Pattern[str], str]]:
"""Confuse validation function that reads regex/string pairs."""
replacements: list[tuple[re.Pattern[str], str]] = []
pattern: str
repl: str
for pattern, repl in config["replace"].get(dict).items():
repl = repl or ""
try:
replacements.append((re.compile(pattern), repl))
except re.error:
raise UserError(
f"malformed regular expression in replace: {pattern}"
)
return replacements
</code_context>
<issue_to_address>
**issue (code-quality):** Explicitly raise from a previous error ([`raise-from-previous-error`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/raise-from-previous-error/))
</issue_to_address>
### Comment 13
<location> `beets/ui/core.py:510` </location>
<code_context>
def split_into_lines(string: str, width_tuple: tuple[int, int, int]):
"""Splits string into a list of substrings at whitespace.
`width_tuple` is a 3-tuple of `(first_width, last_width, middle_width)`.
The first substring has a length not longer than `first_width`, the last
substring has a length not longer than `last_width`, and all other
substrings have a length not longer than `middle_width`.
`string` may contain ANSI codes at word borders.
"""
first_width, middle_width, last_width = width_tuple
words: list[str]
if uncolorize(string) == string:
# No colors in string
words = string.split()
else:
# Use a regex to find escapes and the text within them.
words = []
m: re.Match[str]
for m in ESC_TEXT_REGEX.finditer(string):
# m contains four groups:
# pretext - any text before escape sequence
# esc - intitial escape sequence
# text - text, no escape sequence, may contain spaces
# reset - ASCII colour reset
space_before_text: bool = False
if m.group("pretext") != "":
# Some pretext found, let's handle it
# Add any words in the pretext
words += m.group("pretext").split()
if m.group("pretext")[-1] == " ":
# Pretext ended on a space
space_before_text = True
else:
# Pretext ended mid-word, ensure next word
pass
else:
# pretext empty, treat as if there is a space before
space_before_text = True
if m.group("text")[0] == " ":
# First character of the text is a space
space_before_text = True
# Now, handle the words in the main text:
raw_words: list[str] = m.group("text").split()
if space_before_text:
# Colorize each word with pre/post escapes
# Reconstruct colored words
words += [
f"{m['esc']}{raw_word}{RESET_COLOR}"
for raw_word in raw_words
]
elif raw_words:
# Pretext stops mid-word
if m.group("esc") != RESET_COLOR:
# Add the rest of the current word, with a reset after it
words[-1] += f"{m['esc']}{raw_words[0]}{RESET_COLOR}"
# Add the subsequent colored words:
words += [
f"{m['esc']}{raw_word}{RESET_COLOR}"
for raw_word in raw_words[1:]
]
else:
# Caught a mid-word escape sequence
words[-1] += raw_words[0]
words += raw_words[1:]
if (
m.group("text")[-1] != " "
and m.group("posttext") != ""
and m.group("posttext")[0] != " "
):
# reset falls mid-word
post_text: list[str] = m.group("posttext").split()
words[-1] += post_text[0]
words += post_text[1:]
else:
# Add any words after escape sequence
words += m.group("posttext").split()
result: list[str] = []
next_substr: str = str("")
# Iterate over all words.
previous_fit: bool = False
i: int
for i in range(len(words)): # pyrefly: ignore[bad-assignment]
pot_substr: str
if i == 0:
pot_substr = words[i]
else:
# (optimistically) add the next word to check the fit
pot_substr = " ".join([next_substr, words[i]])
# Find out if the pot(ential)_substr fits into the next substring.
fits_first: bool = (
len(result) == 0 and color_len(pot_substr) <= first_width
)
fits_middle: bool = (
len(result) != 0 and color_len(pot_substr) <= middle_width
)
if fits_first or fits_middle:
# Fitted(!) let's try and add another word before appending
next_substr = pot_substr
previous_fit = True
elif not fits_first and not fits_middle and previous_fit:
# Extra word didn't fit, append what we have
result.append(next_substr)
next_substr = words[i]
previous_fit = color_len(next_substr) <= middle_width
else:
# Didn't fit anywhere
if uncolorize(pot_substr) == pot_substr:
# Simple uncolored string, append a cropped word
if len(result) == 0:
# Crop word by the first_width for the first line
result.append(pot_substr[:first_width])
# add rest of word to next line
next_substr = pot_substr[first_width:]
else:
result.append(pot_substr[:middle_width])
next_substr = pot_substr[middle_width:]
else:
# Colored strings
if len(result) == 0:
this_line, next_line = color_split(pot_substr, first_width)
result.append(this_line)
next_substr = next_line
else:
this_line, next_line = color_split(pot_substr, middle_width)
result.append(this_line)
next_substr = next_line
previous_fit = color_len(next_substr) <= middle_width
# We finished constructing the substrings, but the last substring
# has not yet been added to the result.
result.append(next_substr)
# Also, the length of the last substring was only checked against
# `middle_width`. Append an empty substring as the new last substring if
# the last substring is too long.
if not color_len(next_substr) <= last_width:
result.append("")
return result
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Remove redundant pass statement ([`remove-redundant-pass`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-redundant-pass/))
- Replace if statement with if expression ([`assign-if-exp`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/assign-if-exp/))
- Remove redundant conditional ([`remove-redundant-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-redundant-if/))
- Hoist repeated code outside conditional statement [×2] ([`hoist-statement-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-statement-from-if/))
- Low code quality found in split\_into\_lines - 12% ([`low-code-quality`](https://docs.sourcery.ai/Reference/Default-Rules/comments/low-code-quality/))
<br/><details><summary>Explanation</summary>
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines.
- Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.</details>
</issue_to_address>
### Comment 14
<location> `beets/ui/core.py:650` </location>
<code_context>
def print_column_layout(
indent_str: str,
left: ColumnLayout,
right: ColumnLayout,
separator: str = " -> ",
max_width: int = term_width(),
) -> None:
"""Print left & right data, with separator inbetween
'left' and 'right' have a structure of:
{'prefix':u'','contents':u'','suffix':u'','width':0}
In a column layout the printing will be:
{indent_str}{lhs0}{separator}{rhs0}
{lhs1 / padding }{rhs1}
...
The first line of each column (i.e. {lhs0} or {rhs0}) is:
{prefix}{part of contents}{suffix}
With subsequent lines (i.e. {lhs1}, {rhs1} onwards) being the
rest of contents, wrapped if the width would be otherwise exceeded.
"""
if f"{right['prefix']}{right['contents']}{right['suffix']}" == "":
# No right hand information, so we don't need a separator.
separator = ""
first_line_no_wrap: str = (
f"{indent_str}{left['prefix']}{left['contents']}{left['suffix']}"
f"{separator}{right['prefix']}{right['contents']}{right['suffix']}"
)
if color_len(first_line_no_wrap) < max_width:
# Everything fits, print out line.
print_(first_line_no_wrap)
else:
# Wrap into columns
if "width" not in left or "width" not in right:
# If widths have not been defined, set to share space.
left["width"] = (
max_width - len(indent_str) - color_len(separator)
) // 2
right["width"] = (
max_width - len(indent_str) - color_len(separator)
) // 2
# On the first line, account for suffix as well as prefix
left_width_tuple: tuple[int, int, int] = (
left["width"]
- color_len(left["prefix"])
- color_len(left["suffix"]),
left["width"] - color_len(left["prefix"]),
left["width"] - color_len(left["prefix"]),
)
left_split: list[str] = split_into_lines(
left["contents"], left_width_tuple
)
right_width_tuple: tuple[int, int, int] = (
right["width"]
- color_len(right["prefix"])
- color_len(right["suffix"]),
right["width"] - color_len(right["prefix"]),
right["width"] - color_len(right["prefix"]),
)
right_split: list[str] = split_into_lines(
right["contents"], right_width_tuple
)
max_line_count: int = max(len(left_split), len(right_split))
out: str = ""
i: int
for i in range(max_line_count):
# indentation
out += indent_str
# Prefix or indent_str for line
if i == 0:
out += left["prefix"]
else:
out += indent(color_len(left["prefix"]))
# Line i of left hand side contents.
left_part_len: int
if i < len(left_split):
out += left_split[i]
left_part_len = color_len(left_split[i])
else:
left_part_len = 0
# Padding until end of column.
# Note: differs from original
# column calcs in not -1 afterwards for space
# in track number as that is included in 'prefix'
padding: int = (
left["width"] - color_len(left["prefix"]) - left_part_len
)
# Remove some padding on the first line to display
# length
if i == 0:
padding -= color_len(left["suffix"])
out += indent(padding)
if i == 0:
out += left["suffix"]
# Separator between columns.
if i == 0:
out += separator
else:
out += indent(color_len(separator))
# Right prefix, contents, padding, suffix
if i == 0:
out += right["prefix"]
else:
out += indent(color_len(right["prefix"]))
# Line i of right hand side.
right_part_len: int
if i < len(right_split):
out += right_split[i]
right_part_len = color_len(right_split[i])
else:
right_part_len = 0
# Padding until end of column
padding = (
right["width"] - color_len(right["prefix"]) - right_part_len
)
# Remove some padding on the first line to display
# length
if i == 0:
padding -= color_len(right["suffix"])
out += indent(padding)
# Length in first line
if i == 0:
out += right["suffix"]
# Linebreak, except in the last line.
if i < max_line_count - 1:
out += "\n"
# Constructed all of the columns, now print
print_(out)
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Replace if statement with if expression [×3] ([`assign-if-exp`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/assign-if-exp/))
- Low code quality found in print\_column\_layout - 18% ([`low-code-quality`](https://docs.sourcery.ai/Reference/Default-Rules/comments/low-code-quality/))
<br/><details><summary>Explanation</summary>
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines.
- Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.</details>
</issue_to_address>
### Comment 15
<location> `beets/ui/core.py:935` </location>
<code_context>
def show_model_changes(
new: library.LibModel,
old: library.LibModel | None = None,
fields: Iterable[str] | None = None,
always: bool = False,
print_obj: bool = True,
) -> bool:
"""Given a Model object, print a list of changes from its pristine
version stored in the database. Return a boolean indicating whether
any changes were found.
`old` may be the "original" object to avoid using the pristine
version from the database. `fields` may be a list of fields to
restrict the detection to. `always` indicates whether the object is
always identified, regardless of whether any changes are present.
"""
if not old and new._db:
old = new._db._get(type(new), new.id)
# Keep the formatted views around instead of re-creating them in each
# iteration step
old_fmt: db.FormattedMapping | None = old.formatted() if old else None
new_fmt: db.FormattedMapping = new.formatted()
# Build up lines showing changed fields
field: str
changes: list[str] = []
if old:
for field in old:
# Subset of the fields. Never show mtime.
if field == "mtime" or (fields and field not in fields):
continue
# Detect and show difference for this field.
line: str | None = _field_diff(field, old, old_fmt, new, new_fmt)
if line:
changes.append(f" {field}: {line}")
# New fields.
for field in set(new) - set(old or ()):
if fields and field not in fields:
continue
changes.append(
f" {field}: {colorize('text_highlight', new_fmt[field])}"
)
# Print changes.
if print_obj and (changes or always):
print_(format(old))
if changes:
print_("\n".join(changes))
return bool(changes)
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
- Replace a for append loop with list extend ([`for-append-to-extend`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/for-append-to-extend/))
- Lift code into else after jump in control flow ([`reintroduce-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/reintroduce-else/))
- Remove redundant continue statement ([`remove-redundant-continue`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-redundant-continue/))
</issue_to_address>
### Comment 16
<location> `beets/ui/core.py:1028-1036` </location>
<code_context>
def _store_dict(
option: optparse.Option,
opt_str: str,
value: str,
parser: optparse.OptionParser,
) -> None:
"""Custom action callback to parse options which have ``key=value``
pairs as values. All such pairs passed for this option are
aggregated into a dictionary.
"""
dest: str = option.dest or ""
option_values: dict[str, str] | None = getattr(parser.values, dest, None)
if option_values is None:
# This is the first supplied ``key=value`` pair of option.
# Initialize empty dictionary and get a reference to it.
setattr(parser.values, dest, {})
option_values = getattr(parser.values, dest)
try:
key: str
key, value = value.split("=", 1)
if not (key and value):
raise ValueError
except ValueError:
raise UserError(
f"supplied argument `{value}' is not of the form `key=value'"
)
option_values[key] = value
</code_context>
<issue_to_address>
**issue (code-quality):** Explicitly raise from a previous error ([`raise-from-previous-error`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/raise-from-previous-error/))
</issue_to_address>
### Comment 17
<location> `beets/ui/core.py:1112-1119` </location>
<code_context>
def _set_format(
self,
option: optparse.Option,
opt_str: str,
value: str,
parser: CommonOptionsParser,
target: type[library.Album | library.Item] | None = None,
fmt: str | None = None,
store_true: bool = False,
) -> None:
"""Internal callback that sets the correct format while parsing CLI
arguments.
"""
if store_true and option.dest:
setattr(parser.values, option.dest, True)
# Use the explicitly specified format, or the string from the option.
value = fmt or value or ""
if parser.values is None:
parser.values = optparse.Values()
parser.values.format = value
if target:
config[target._format_config_key].set(value)
return
if self._album_flags:
if parser.values.album:
target = library.Album
else:
# the option is either missing either not parsed yet
if self._album_flags & set(parser.rargs or ()):
target = library.Album
else:
target = library.Item
config[target._format_config_key].set(value)
else:
config[library.Item._format_config_key].set(value)
config[library.Album._format_config_key].set(value)
</code_context>
<issue_to_address>
**issue (code-quality):** Merge else clause's nested if statement into elif ([`merge-else-if-into-elif`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/merge-else-if-into-elif/))
</issue_to_address>
### Comment 18
<location> `beets/ui/core.py:1295-1296` </location>
<code_context>
@override
def format_help(
self, formatter: optparse.HelpFormatter | None = None
) -> str:
# Get the original help message, to which we will append.
out: str = super().format_help(formatter)
if formatter is None:
formatter = self.formatter
# Subcommands header.
result: list[str] = ["\n"]
result.append(formatter.format_heading("Commands"))
formatter.indent()
# Generate the display names (including aliases).
# Also determine the help position.
disp_names: list[str] = []
help_position: int = 0
subcommands: list[Subcommand] = [
c for c in self.subcommands if not c.hide
]
subcommands.sort(key=lambda c: c.name)
name: str
subcommand: Subcommand
for subcommand in subcommands:
name = subcommand.name
if subcommand.aliases:
name += f" ({', '.join(subcommand.aliases)})"
disp_names.append(name)
# Set the help position based on the max width.
proposed_help_position: int = (
len(name) + formatter.current_indent + 2
)
if proposed_help_position <= formatter.max_help_position:
help_position = max(help_position, proposed_help_position)
# Add each subcommand to the output.
for subcommand, name in zip(subcommands, disp_names):
# Lifted directly from optparse.py.
name_width: int = help_position - formatter.current_indent - 2
indent_first: int
if len(name) > name_width:
name = f"{' ' * formatter.current_indent}{name}\n"
indent_first = help_position
else:
name = f"{' ' * formatter.current_indent}{name:<{name_width}}\n"
indent_first = 0
result.append(name)
help_width: int = formatter.width - help_position
help_lines: list[str] = textwrap.wrap(subcommand.help, help_width)
help_line: str = help_lines[0] if help_lines else ""
result.append(f"{' ' * indent_first}{help_line}\n")
result.extend(
[f"{' ' * help_position}{line}\n" for line in help_lines[1:]]
)
formatter.dedent()
# Concatenate the original help message with the subcommand
# list.
return f"{out}{''.join(result)}"
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge append into list declaration ([`merge-list-append`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/merge-list-append/))
```suggestion
result: list[str] = ["\n", formatter.format_heading("Commands")]
```
</issue_to_address>
### Comment 19
<location> `beets/ui/core.py:1353-1356` </location>
<code_context>
def _subcommand_for_name(self, name: str) -> Subcommand | None:
"""Return the subcommand in self.subcommands matching the
given name. The name may either be the name of a subcommand or
an alias. If no subcommand matches, returns None.
"""
subcommand: Subcommand
for subcommand in self.subcommands:
if name == subcommand.name or name in subcommand.aliases:
return subcommand
return None
</code_context>
<issue_to_address>
**suggestion (code-quality):** Use the built-in function `next` instead of a for-loop ([`use-next`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-next/))
```suggestion
return next(
(
subcommand
for subcommand in self.subcommands
if name == subcommand.name or name in subcommand.aliases
),
None,
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return ANSI_CODE_REGEX.sub("", colored_text) | ||
|
|
||
|
|
||
| def color_split(colored_text: str, index: int): |
Contributor
There was a problem hiding this comment.
suggestion: Function color_split lacks a return type annotation.
Please add a return type annotation to color_split to enhance code clarity and support static analysis tools.
Suggested change
| def color_split(colored_text: str, index: int): | |
| def color_split(colored_text: str, index: int) -> tuple[str, str]: |
Comment on lines
+173
to
+176
| if part == RESET_COLOR: | ||
| found_color_code = None | ||
| else: | ||
| found_color_code = part |
Contributor
There was a problem hiding this comment.
suggestion (code-quality): Replace if statement with if expression (assign-if-exp)
Suggested change
| if part == RESET_COLOR: | |
| found_color_code = None | |
| else: | |
| found_color_code = part | |
| found_color_code = None if part == RESET_COLOR else part |
Comment on lines
+256
to
+259
| if config["ui"]["color"]: | ||
| return _colordiff(a, b) | ||
| else: | ||
| return str(a), str(b) |
Contributor
There was a problem hiding this comment.
suggestion (code-quality): Replace if statement with if expression (assign-if-exp)
Suggested change
| if config["ui"]["color"]: | |
| return _colordiff(a, b) | |
| else: | |
| return str(a), str(b) | |
| return _colordiff(a, b) if config["ui"]["color"] else (str(a), str(b)) |
| ". Please set the VISUAL (or EDITOR) environment variable" | ||
| ) | ||
| raise ui.UserError(message) | ||
| raise UserError(message) |
Contributor
There was a problem hiding this comment.
suggestion (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
Suggested change
| raise UserError(message) | |
| raise UserError(message) from exc |
Comment on lines
+88
to
+90
| # Configured override? | ||
| encoding: str = config["terminal_encoding"].get() | ||
| if encoding: |
Contributor
There was a problem hiding this comment.
suggestion (code-quality): Use named expression to simplify assignment and conditional (use-named-expression)
Suggested change
| # Configured override? | |
| encoding: str = config["terminal_encoding"].get() | |
| if encoding: | |
| if encoding := config["terminal_encoding"].get(): |
Comment on lines
+304
to
+308
| if numrange: | ||
| default = numrange[0] | ||
| else: | ||
| default = display_letters[0].lower() | ||
|
|
Contributor
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Replace if statement with if expression (
assign-if-exp) - Low code quality found in input_options - 4% (
low-code-quality)
Suggested change
| if numrange: | |
| default = numrange[0] | |
| else: | |
| default = display_letters[0].lower() | |
| default = numrange[0] if numrange else display_letters[0].lower() |
Explanation
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
| return width | ||
|
|
||
|
|
||
| def split_into_lines(string: str, width_tuple: tuple[int, int, int]): |
Contributor
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Remove redundant pass statement (
remove-redundant-pass) - Replace if statement with if expression (
assign-if-exp) - Remove redundant conditional (
remove-redundant-if) - Hoist repeated code outside conditional statement [×2] (
hoist-statement-from-if) - Low code quality found in split_into_lines - 12% (
low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
| return result | ||
|
|
||
|
|
||
| def print_column_layout( |
Contributor
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Replace if statement with if expression [×3] (
assign-if-exp) - Low code quality found in print_column_layout - 18% (
low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
Comment on lines
+1295
to
+1296
| result: list[str] = ["\n"] | ||
| result.append(formatter.format_heading("Commands")) |
Contributor
There was a problem hiding this comment.
suggestion (code-quality): Merge append into list declaration (merge-list-append)
Suggested change
| result: list[str] = ["\n"] | |
| result.append(formatter.format_heading("Commands")) | |
| result: list[str] = ["\n", formatter.format_heading("Commands")] |
Comment on lines
+1353
to
+1356
| for subcommand in self.subcommands: | ||
| if name == subcommand.name or name in subcommand.aliases: | ||
| return subcommand | ||
| return None |
Contributor
There was a problem hiding this comment.
suggestion (code-quality): Use the built-in function next instead of a for-loop (use-next)
Suggested change
| for subcommand in self.subcommands: | |
| if name == subcommand.name or name in subcommand.aliases: | |
| return subcommand | |
| return None | |
| return next( | |
| ( | |
| subcommand | |
| for subcommand in self.subcommands | |
| if name == subcommand.name or name in subcommand.aliases | |
| ), | |
| None, | |
| ) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
Fixes import cycle in ui
To Do