Refactor ui/commands.py monolith into modular structure
#6119
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
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the monolithic ui/commands.py file (2000+ lines) into a modular structure with one file per command. The refactoring splits commands into separate modules under ui/commands/, reorganizes utility functions into _utils.py, and moves import-related code into its own import_/ subdirectory with session.py and display.py. Tests are similarly reorganized into command-specific files under test/ui/commands/.
Key changes:
- Commands modularized into individual files (
list.py,move.py,update.py,remove.py, etc.) - Import command split into
import_/subdirectory withsession.pyanddisplay.py - Utility functions moved to
_utils.py
Reviewed Changes
Copilot reviewed 35 out of 40 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
beets/ui/commands/__init__.py |
New module entry point that imports and exports all command subcommands |
beets/ui/commands/_utils.py |
Utility functions extracted from monolithic commands file |
beets/ui/commands/import_/session.py |
Import session logic extracted into dedicated module |
beets/ui/commands/import_/display.py |
Import display functions extracted into dedicated module |
beets/ui/commands/*.py |
Individual command modules (list, move, update, remove, etc.) |
test/ui/commands/*.py |
Command-specific test files matching new structure |
test/ui/test_ui.py |
General UI tests moved from test/test_ui.py |
beets/ui/commands.py |
Removed monolithic file |
test/test_ui.py |
Removed monolithic test file |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| """Modifies matching items according to user-specified assignments and | ||
| deletions. | ||
| `mods` is a dictionary of field and value pairse indicating |
There was a problem hiding this comment.
Corrected spelling of 'pairse' to 'pairs'
Suggested change
| `mods` is a dictionary of field and value pairse indicating | |
| `mods` is a dictionary of field and value pairs indicating |
| items = self.lib.items() | ||
| assert len(list(items)) == 1 | ||
| # There is probably no guarantee that the items are queried in any | ||
| # spcecific order, thus just ensure that exactly one was removed. |
There was a problem hiding this comment.
Corrected spelling of 'spcecific' to 'specific'
Suggested change
| # spcecific order, thus just ensure that exactly one was removed. | |
| # specific order, thus just ensure that exactly one was removed. |
| def completion_script(commands): | ||
| """Yield the full completion shell script as strings. | ||
| ``commands`` is alist of ``ui.Subcommand`` instances to generate |
There was a problem hiding this comment.
Corrected spacing: 'alist' should be 'a list'
Suggested change
| ``commands`` is alist of ``ui.Subcommand`` instances to generate | |
| ``commands`` is a list of ``ui.Subcommand`` instances to generate |
| manually resolving duplicates during import. | ||
| `items` is a list of `Item` objects. `singleton` indicates whether | ||
| this is an album or single-item import (if the latter, them `items` |
There was a problem hiding this comment.
Corrected spelling of 'them' to 'then'
Suggested change
| this is an album or single-item import (if the latter, them `items` | |
| this is an album or single-item import (if the latter, then `items` |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider renaming internal modules under ui/commands (e.g. display, session, _utils) to start with an underscore and explicitly define all in init.py so only the public Subcommand interfaces are exposed.
- Rather than adding every move-refactor commit to .git-blame-ignore-revs, group file relocations into a single commit per module or use a path‐based ignore pattern to avoid spammy entries.
- Add deprecation wrappers in ui/commands/init.py for any relocated classes or functions to provide clear warnings for downstream imports of old symbols.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider renaming internal modules under ui/commands (e.g. display, session, _utils) to start with an underscore and explicitly define __all__ in __init__.py so only the public Subcommand interfaces are exposed.
- Rather than adding every move-refactor commit to .git-blame-ignore-revs, group file relocations into a single commit per module or use a path‐based ignore pattern to avoid spammy entries.
- Add deprecation wrappers in ui/commands/__init__.py for any relocated classes or functions to provide clear warnings for downstream imports of old symbols.
## Individual Comments
### Comment 1
<location> `beets/ui/commands/update.py:24-28` </location>
<code_context>
- """
- with lib.transaction():
- items, _ = _do_query(lib, query, album)
- if move and fields is not None and "path" not in fields:
- # Special case: if an item needs to be moved, the path field has to
- # updated; otherwise the new path will not be reflected in the
- # database.
- fields.append("path")
- if fields is None:
- # no fields were provided, update all media fields
</code_context>
<issue_to_address>
**issue (bug_risk):** Appending 'path' to fields may cause issues if fields is a set.
If 'fields' is a set, using 'append' will fail. Use 'add' for sets or ensure 'fields' is always a list.
</issue_to_address>
### Comment 2
<location> `beets/ui/commands/move.py:27` </location>
<code_context>
- dest is None, then the library's base directory is used, making the
- command "consolidate" files.
- """
- dest = os.fsencode(dest_path) if dest_path else dest_path
- items, albums = _do_query(lib, query, album, False)
- objs = albums if album else items
</code_context>
<issue_to_address>
**suggestion (bug_risk):** os.fsencode may not be necessary if dest_path is already bytes.
os.fsencode will raise a TypeError if dest_path is already bytes. Please check the type before encoding or ensure dest_path is consistently a str or bytes throughout the codebase.
```suggestion
if dest_path:
dest = os.fsencode(dest_path) if isinstance(dest_path, str) else dest_path
else:
dest = dest_path
```
</issue_to_address>
### Comment 3
<location> `test/ui/commands/test_modify.py:180-179` </location>
<code_context>
- assert "flexattr" not in item
-
- @unittest.skip("not yet implemented")
- def test_delete_initial_key_tag(self):
- item = self.lib.items().get()
- item.initial_key = "C#m"
</code_context>
<issue_to_address>
**suggestion (testing):** Skipped test for deleting initial_key tag.
If deleting the initial_key tag should be supported, please implement the test and functionality, or clarify why it is intentionally omitted.
Suggested implementation:
```python
def test_delete_initial_key_tag(self):
item = self.lib.items().get()
item.initial_key = "C#m"
item.write()
item.store()
mediafile = MediaFile(syspath(item.path))
assert mediafile.initial_key == "C#m"
self.modify("initial_key!")
mediafile = MediaFile(syspath(item.path))
assert mediafile.initial_key is None
```
If the `modify("initial_key!")` command does not actually remove the `initial_key` tag from the item and its media file, you will need to implement or verify that functionality in the codebase where the `modify` method is defined. Ensure that it supports removing the `initial_key` tag when the exclamation mark syntax is used.
</issue_to_address>
### Comment 4
<location> `beets/ui/commands/import_/display.py:253` </location>
<code_context>
cur_length0 = item.length if item.length else 0
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace if-expression with `or` ([`or-if-exp-identity`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/or-if-exp-identity))
```suggestion
cur_length0 = item.length or 0
```
<br/><details><summary>Explanation</summary>Here we find ourselves setting a value if it evaluates to `True`, and otherwise
using a default.
The 'After' case is a bit easier to read and avoids the duplication of
`input_currency`.
It works because the left-hand side is evaluated first. If it evaluates to
true then `currency` will be set to this and the right-hand side will not be
evaluated. If it evaluates to false the right-hand side will be evaluated and
`currency` will be set to `DEFAULT_CURRENCY`.
</details>
</issue_to_address>
### Comment 5
<location> `beets/ui/commands/import_/display.py:254` </location>
<code_context>
new_length0 = track_info.length if track_info.length else 0
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace if-expression with `or` ([`or-if-exp-identity`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/or-if-exp-identity))
```suggestion
new_length0 = track_info.length or 0
```
<br/><details><summary>Explanation</summary>Here we find ourselves setting a value if it evaluates to `True`, and otherwise
using a default.
The 'After' case is a bit easier to read and avoids the duplication of
`input_currency`.
It works because the left-hand side is evaluated first. If it evaluates to
true then `currency` will be set to this and the right-hand side will not be
evaluated. If it evaluates to false the right-hand side will be evaluated and
`currency` will be set to `DEFAULT_CURRENCY`.
</details>
</issue_to_address>
### Comment 6
<location> `test/ui/commands/test_completion.py:30-31` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 7
<location> `test/ui/commands/test_completion.py:37-42` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 8
<location> `test/ui/commands/test_completion.py:38-40` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 9
<location> `test/ui/commands/test_import.py:26-51` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 10
<location> `test/ui/commands/test_modify.py:95-98` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 11
<location> `test/ui/commands/test_modify.py:95` </location>
<code_context>
for i in range(0, 10):
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace range(0, x) with range(x) ([`remove-zero-from-range`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/remove-zero-from-range))
```suggestion
for i in range(10):
```
<br/><details><summary>Explanation</summary>The default starting value for a call to `range()` is 0, so it is unnecessary to
explicitly define it. This refactoring removes such zeros, slightly shortening
the code.
</details>
</issue_to_address>
### Comment 12
<location> `test/ui/commands/test_modify.py:108-111` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 13
<location> `test/ui/commands/test_modify.py:108` </location>
<code_context>
for i in range(0, 3):
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace range(0, x) with range(x) ([`remove-zero-from-range`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/remove-zero-from-range))
```suggestion
for i in range(3):
```
<br/><details><summary>Explanation</summary>The default starting value for a call to `range()` is 0, so it is unnecessary to
explicitly define it. This refactoring removes such zeros, slightly shortening
the code.
</details>
</issue_to_address>
### Comment 14
<location> `test/ui/commands/test_modify.py:114-117` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 15
<location> `test/ui/commands/test_remove.py:49-50` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 16
<location> `test/ui/commands/test_remove.py:71-72` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 17
<location> `test/ui/test_ui.py:45-46` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 18
<location> `test/ui/test_ui.py:53-54` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 19
<location> `test/ui/test_ui.py:67-70` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 20
<location> `test/ui/test_ui.py:71-74` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 21
<location> `beets/test/_common.py:109` </location>
<code_context>
def import_session(lib=None, loghandler=None, paths=[], query=[], cli=False):
cls = (
commands._import.session.TerminalImportSession
if cli
else importer.ImportSession
)
return cls(lib, loghandler, paths, query)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace mutable default arguments with None ([`default-mutable-arg`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/default-mutable-arg/))
```suggestion
def import_session(lib=None, loghandler=None, paths=None, query=None, cli=False):
if paths is None:
paths = []
if query is None:
query = []
```
</issue_to_address>
### Comment 22
<location> `beets/ui/commands/config.py:59` </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 ui.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 ui.UserError(message) from exc
```
</issue_to_address>
### Comment 23
<location> `beets/ui/commands/help.py:17-19` </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 ui.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 24
<location> `beets/ui/commands/import_/__init__.py:26-32` </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 ui.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 ui.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 25
<location> `beets/ui/commands/import_/display.py:72` </location>
<code_context>
def show_match_header(self):
"""Print out a 'header' identifying the suggested match (album name,
artist name,...) and summarizing the changes that would be made should
the user accept the match.
"""
# Print newline at beginning of change block.
ui.print_("")
# 'Match' line and similarity.
ui.print_(
f"{self.indent_header}Match ({dist_string(self.match.distance)}):"
)
if isinstance(self.match.info, autotag.hooks.AlbumInfo):
# Matching an album - print that
artist_album_str = (
f"{self.match.info.artist} - {self.match.info.album}"
)
else:
# Matching a single track
artist_album_str = (
f"{self.match.info.artist} - {self.match.info.title}"
)
ui.print_(
self.indent_header
+ dist_colorize(artist_album_str, self.match.distance)
)
# Penalties.
penalties = penalty_string(self.match.distance)
if penalties:
ui.print_(f"{self.indent_header}{penalties}")
# Disambiguation.
disambig = disambig_string(self.match.info)
if disambig:
ui.print_(f"{self.indent_header}{disambig}")
# Data URL.
if self.match.info.data_url:
url = ui.colorize("text_faint", f"{self.match.info.data_url}")
ui.print_(f"{self.indent_header}{url}")
</code_context>
<issue_to_address>
**issue (code-quality):** Use named expression to simplify assignment and conditional [×2] ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
</issue_to_address>
### Comment 26
<location> `beets/ui/commands/import_/display.py:124` </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 = ui.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:
ui.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 = ui.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:
ui.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 = ui.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:
ui.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 27
<location> `beets/ui/commands/import_/display.py:294-304` </location>
<code_context>
def make_line(self, item, track_info):
"""Extract changes from item -> new TrackInfo object, and colorize
appropriately. Returns (lhs, rhs) for column printing.
"""
# Track titles.
lhs_title, rhs_title, diff_title = self.make_track_titles(
item, track_info
)
# Track number change.
lhs_track, rhs_track, diff_track = self.make_track_numbers(
item, track_info
)
# Length change.
lhs_length, rhs_length, diff_length = self.make_track_lengths(
item, track_info
)
changed = diff_title or diff_track or diff_length
# Construct lhs and rhs dicts.
# Previously, we printed the penalties, however this is no longer
# the case, thus the 'info' dictionary is unneeded.
# penalties = penalty_string(self.match.distance.tracks[track_info])
lhs = {
"prefix": f"{self.changed_prefix if changed else '*'} {lhs_track} ",
"contents": lhs_title,
"suffix": f" {lhs_length}",
}
rhs = {"prefix": "", "contents": "", "suffix": ""}
if not changed:
# Only return the left side, as nothing changed.
return (lhs, rhs)
else:
# Construct a dictionary for the "changed to" side
rhs = {
"prefix": f"{rhs_track} ",
"contents": rhs_title,
"suffix": f" {rhs_length}",
}
return (lhs, rhs)
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Swap if/else to remove empty if body ([`remove-pass-body`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-pass-body/))
- Hoist repeated code outside conditional statement ([`hoist-statement-from-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/hoist-statement-from-if/))
</issue_to_address>
### Comment 28
<location> `beets/ui/commands/import_/display.py:401-405` </location>
<code_context>
def show_match_tracks(self):
"""Print out the tracks of the match, summarizing changes the match
suggests for them.
"""
# Tracks.
# match is an AlbumMatch NamedTuple, mapping is a dict
# Sort the pairs by the track_info index (at index 1 of the NamedTuple)
pairs = list(self.match.mapping.items())
pairs.sort(key=lambda item_and_track_info: item_and_track_info[1].index)
# Build up LHS and RHS for track difference display. The `lines` list
# contains `(left, right)` tuples.
lines = []
medium = disctitle = None
for item, track_info in pairs:
# If the track is the first on a new medium, show medium
# number and title.
if medium != track_info.medium or disctitle != track_info.disctitle:
# Create header for new medium
header = self.make_medium_info_line(track_info)
if header != "":
# Print tracks from previous medium
self.print_tracklist(lines)
lines = []
ui.print_(f"{self.indent_detail}{header}")
# Save new medium details for future comparison.
medium, disctitle = track_info.medium, track_info.disctitle
# Construct the line tuple for the track.
left, right = self.make_line(item, track_info)
if right["contents"] != "":
lines.append((left, right))
else:
if config["import"]["detail"]:
lines.append((left, right))
self.print_tracklist(lines)
# Missing and unmatched tracks.
if self.match.extra_tracks:
ui.print_(
"Missing tracks"
f" ({len(self.match.extra_tracks)}/{len(self.match.info.tracks)} -"
f" {len(self.match.extra_tracks) / len(self.match.info.tracks):.1%}):"
)
for track_info in self.match.extra_tracks:
line = f" ! {track_info.title} (#{self.format_index(track_info)})"
if track_info.length:
line += f" ({human_seconds_short(track_info.length)})"
ui.print_(ui.colorize("text_warning", line))
if self.match.extra_items:
ui.print_(f"Unmatched tracks ({len(self.match.extra_items)}):")
for item in self.match.extra_items:
line = f" ! {item.title} (#{self.format_index(item)})"
if item.length:
line += f" ({human_seconds_short(item.length)})"
ui.print_(ui.colorize("text_warning", line))
</code_context>
<issue_to_address>
**issue (code-quality):** Merge duplicate blocks in conditional ([`merge-duplicate-blocks`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/merge-duplicate-blocks/))
</issue_to_address>
### Comment 29
<location> `beets/ui/commands/import_/session.py:318` </location>
<code_context>
def summarize_items(items, singleton):
"""Produces a brief summary line describing a set of items. Used for
manually resolving duplicates during import.
`items` is a list of `Item` objects. `singleton` indicates whether
this is an album or single-item import (if the latter, them `items`
should only have one element).
"""
summary_parts = []
if not singleton:
summary_parts.append(f"{len(items)} items")
format_counts = {}
for item in items:
format_counts[item.format] = format_counts.get(item.format, 0) + 1
if len(format_counts) == 1:
# A single format.
summary_parts.append(items[0].format)
else:
# Enumerate all the formats by decreasing frequencies:
for fmt, count in sorted(
format_counts.items(),
key=lambda fmt_and_count: (-fmt_and_count[1], fmt_and_count[0]),
):
summary_parts.append(f"{fmt} {count}")
if items:
average_bitrate = sum([item.bitrate for item in items]) / len(items)
total_duration = sum([item.length for item in items])
total_filesize = sum([item.filesize for item in items])
summary_parts.append(f"{int(average_bitrate / 1000)}kbps")
if items[0].format == "FLAC":
sample_bits = (
f"{round(int(items[0].samplerate) / 1000, 1)}kHz"
f"/{items[0].bitdepth} bit"
)
summary_parts.append(sample_bits)
summary_parts.append(human_seconds_short(total_duration))
summary_parts.append(human_bytes(total_filesize))
return ", ".join(summary_parts)
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Merge consecutive list appends into a single extend ([`merge-list-appends-into-extend`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/merge-list-appends-into-extend/))
- Extract code out into function ([`extract-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-method/))
- Replace unneeded comprehension with generator [×3] ([`comprehension-to-generator`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/comprehension-to-generator/))
</issue_to_address>
### Comment 30
<location> `beets/ui/commands/import_/session.py:342` </location>
<code_context>
def _summary_judgment(rec):
"""Determines whether a decision should be made without even asking
the user. This occurs in quiet mode and when an action is chosen for
NONE recommendations. Return None if the user should be queried.
Otherwise, returns an action. May also print to the console if a
summary judgment is made.
"""
if config["import"]["quiet"]:
if rec == Recommendation.strong:
return importer.Action.APPLY
else:
action = config["import"]["quiet_fallback"].as_choice(
{
"skip": importer.Action.SKIP,
"asis": importer.Action.ASIS,
}
)
elif config["import"]["timid"]:
return None
elif rec == Recommendation.none:
action = config["import"]["none_rec_action"].as_choice(
{
"skip": importer.Action.SKIP,
"asis": importer.Action.ASIS,
"ask": None,
}
)
else:
return None
if action == importer.Action.SKIP:
ui.print_("Skipping.")
elif action == importer.Action.ASIS:
ui.print_("Importing as-is.")
return action
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Merge duplicate blocks in conditional ([`merge-duplicate-blocks`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/merge-duplicate-blocks/))
- Remove redundant conditional [×2] ([`remove-redundant-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-redundant-if/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+24
to
+28
| if move and fields is not None and "path" not in fields: | ||
| # Special case: if an item needs to be moved, the path field has to | ||
| # updated; otherwise the new path will not be reflected in the | ||
| # database. | ||
| fields.append("path") |
There was a problem hiding this comment.
issue (bug_risk): Appending 'path' to fields may cause issues if fields is a set.
If 'fields' is a set, using 'append' will fail. Use 'add' for sets or ensure 'fields' is always a list.
| dest is None, then the library's base directory is used, making the | ||
| command "consolidate" files. | ||
| """ | ||
| dest = os.fsencode(dest_path) if dest_path else dest_path |
There was a problem hiding this comment.
suggestion (bug_risk): os.fsencode may not be necessary if dest_path is already bytes.
os.fsencode will raise a TypeError if dest_path is already bytes. Please check the type before encoding or ensure dest_path is consistently a str or bytes throughout the codebase.
Suggested change
| dest = os.fsencode(dest_path) if dest_path else dest_path | |
| if dest_path: | |
| dest = os.fsencode(dest_path) if isinstance(dest_path, str) else dest_path | |
| else: | |
| dest = dest_path |
| item = self.lib.items().get() | ||
| assert "flexattr" not in item | ||
|
|
||
| @unittest.skip("not yet implemented") |
There was a problem hiding this comment.
suggestion (testing): Skipped test for deleting initial_key tag.
If deleting the initial_key tag should be supported, please implement the test and functionality, or clarify why it is intentionally omitted.
Suggested implementation:
def test_delete_initial_key_tag(self):
item = self.lib.items().get()
item.initial_key = "C#m"
item.write()
item.store()
mediafile = MediaFile(syspath(item.path))
assert mediafile.initial_key == "C#m"
self.modify("initial_key!")
mediafile = MediaFile(syspath(item.path))
assert mediafile.initial_key is NoneIf the modify("initial_key!") command does not actually remove the initial_key tag from the item and its media file, you will need to implement or verify that functionality in the codebase where the modify method is defined. Ensure that it supports removing the initial_key tag when the exclamation mark syntax is used.
| highlight_color = "text_highlight_minor" | ||
|
|
||
| # Handle nonetype lengths by setting to 0 | ||
| cur_length0 = item.length if item.length else 0 |
There was a problem hiding this comment.
suggestion (code-quality): Replace if-expression with or (or-if-exp-identity)
Suggested change
| cur_length0 = item.length if item.length else 0 | |
| cur_length0 = item.length or 0 |
Explanation
Here we find ourselves setting a value if it evaluates toTrue, and otherwiseusing a default.
The 'After' case is a bit easier to read and avoids the duplication of
input_currency.
It works because the left-hand side is evaluated first. If it evaluates to
true then currency will be set to this and the right-hand side will not be
evaluated. If it evaluates to false the right-hand side will be evaluated and
currency will be set to DEFAULT_CURRENCY.
|
|
||
| # Handle nonetype lengths by setting to 0 | ||
| cur_length0 = item.length if item.length else 0 | ||
| new_length0 = track_info.length if track_info.length else 0 |
There was a problem hiding this comment.
suggestion (code-quality): Replace if-expression with or (or-if-exp-identity)
Suggested change
| new_length0 = track_info.length if track_info.length else 0 | |
| new_length0 = track_info.length or 0 |
Explanation
Here we find ourselves setting a value if it evaluates toTrue, and otherwiseusing a default.
The 'After' case is a bit easier to read and avoids the duplication of
input_currency.
It works because the left-hand side is evaluated first. If it evaluates to
true then currency will be set to this and the right-hand side will not be
evaluated. If it evaluates to false the right-hand side will be evaluated and
currency will be set to DEFAULT_CURRENCY.
Comment on lines
+49
to
+50
| for s in ("s", "y", "n"): | ||
| self.io.addinput(s) |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
Comment on lines
+71
to
+72
| for s in ("s", "y", "n"): | ||
| self.io.addinput(s) |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
|
|
||
|
|
||
| # Dummy import session. | ||
| def import_session(lib=None, loghandler=None, paths=[], query=[], cli=False): |
There was a problem hiding this comment.
suggestion (code-quality): Replace mutable default arguments with None (default-mutable-arg)
Suggested change
| def import_session(lib=None, loghandler=None, paths=[], query=[], cli=False): | |
| def import_session(lib=None, loghandler=None, paths=None, query=None, cli=False): | |
| if paths is None: | |
| paths = [] | |
| if query is None: | |
| query = [] |
| message += ( | ||
| ". Please set the VISUAL (or EDITOR) environment variable" | ||
| ) | ||
| raise ui.UserError(message) |
There was a problem hiding this comment.
suggestion (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
Suggested change
| raise ui.UserError(message) | |
| raise ui.UserError(message) from exc |
Comment on lines
+26
to
+32
| try: | ||
| loghandler = logging.FileHandler(logpath, encoding="utf-8") | ||
| except OSError: | ||
| raise ui.UserError( | ||
| "Could not open log file for writing:" | ||
| f" {displayable_path(logpath)}" | ||
| ) |
There was a problem hiding this comment.
issue (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6119 +/- ##
==========================================
+ Coverage 66.98% 67.10% +0.11%
==========================================
Files 118 134 +16
Lines 18191 18245 +54
Branches 3079 3079
==========================================
+ Hits 12186 12243 +57
+ Misses 5346 5344 -2
+ Partials 659 658 -1
🚀 New features to boost your workflow:
|
amogus07
reviewed
Oct 24, 2025
amogus07
reviewed
Oct 25, 2025
|
Well done, this must have required a significant time investment to undertake!! <3 Had a quick glance and I see a circular import issue: $ grepr 'from.*ui$' beets
beets/ui/commands/import_/__init__.py:from beets import config, logging, plugins, ui
beets/ui/commands/import_/display.py:from beets import autotag, config, logging, ui
beets/ui/commands/import_/session.py:from beets import autotag, config, importer, logging, plugins, ui
beets/ui/commands/_utils.py:from beets import ui
beets/ui/commands/completion.py:from beets import library, logging, plugins, ui
beets/ui/commands/fields.py:from beets import library, ui
beets/ui/commands/help.py:from beets import ui
beets/ui/commands/list.py:from beets import ui
beets/ui/commands/modify.py:from beets import library, ui
beets/ui/commands/remove.py:from beets import ui
beets/ui/commands/stats.py:from beets import logging, ui
beets/ui/commands/update.py:from beets import library, logging, ui
beets/ui/commands/version.py:from beets import plugins, ui
beets/ui/commands/write.py:from beets import library, logging, ui |
How do you recon we fix these? Intriguing as this doesn't appear to be issue for the test runs 🤔 |
|
This was not necessarily what I meant. I want to keep the efforts here focused on the commands.py file. Also changing things around in the ui base layer seems a bit out of scope here. The major surprise for me is that this does not introduce a runtime error and I would like to understand why. |
It's because the modules in commands don't actually import anything from ui, they just get a reference to it. By the time the functions that use attributes of the module are called, it happens to have fully initialized, which obscures the problem |
|
ui.commands is only imported in _stetup and _raw_main. Those happen to be below everything the commands depend on |
|
Than this does not seem like an actual issue here. As this PR only restructures the module and the imports where handled identically beforehand the problem was existing beforehand too. @snejus Let's omit this here for now and keep this focused on the restructure. We can fix this in a followup more easily anyways. |
|
import cycles don't cause errors directly but can lead attribute access errors |
|
@semohr so I can expect this to get merged and build on top right? Then after this gets merged, I would be able to submit my pr directly to master as a followup |
|
Yeah, I guess we will merge this in one form or another. Feel free to base your upcomming PR on this branch. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
This one’s a big one 🎣 Proceed with care and a bit of time ;)
The
ui/commands.pyfile had grown into an unwieldy monolith (2000+ lines) over time, so this PR breaks it apart into a modular structure i.e. one file per command, plus some cleanup and reorganization along the way.What changed
Every command (
help,list,move,update,remove, etc.) now lives in its own file underui/commands/.commands.pyconverted intocommands/__init__.pyfor better import handling.importcommand (and related helpers) moved into its own folder:importer/session.pyfor import session logicimporter/display.pyfor display-related functionsReview notes
Try to look at one commit at a time - the changes are organized logically across commits to make the refactoring easier to follow. The source code stayed mostly unchanged.
Open questions
How should we approach deprecations? While we could re-export all functions and classes that aren’t prefixed with an underscore, most of the module’s functions feel effectively private, so we might want to consider limiting the public API and phasing out anything that shouldn’t be used externally.
How do we want to add this to the
.git-blame-ignore-revsfile? I feel like putting all commits into it might be a bit spammy. There has to be a better way here.