-
-
Notifications
You must be signed in to change notification settings - Fork 596
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: some fixing on file browser, windows drives browsing, and windows share #8719
Conversation
Warning Rate Limit Exceeded@miigotu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 47 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates focus on enhancing code quality and consistency across the SickChill project. These changes include refactoring for better readability and maintainability, such as adopting Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
sickchill/gui/slick/js/browser.js
Outdated
@@ -183,7 +194,9 @@ | |||
// If the text field is empty, and we're given a key then populate it with the last browsed value from localStorage | |||
try { | |||
hasLocalStorage = Boolean(localStorage.getItem); | |||
} catch {} | |||
} catch (e) { | |||
console.log(e) |
Check notice
Code scanning / CodeQL
Semicolon insertion
sickchill/oldbeard/browser.py
Outdated
# for name, share in settings.WINDOWS_SHARES.items(): | ||
# if name and share.get('server') and share.get('path'): | ||
# entries.append({"name": name, "path": "\\\\{server}\{path}".format(server=share["server"], path=share["path"])}) | ||
# elif share.get('server'): | ||
# entries.append({"name": name, "path": "\\\\{server}\{path}".format(server=share["server"], path=share["path"])}) |
Check notice
Code scanning / CodeQL
Commented-out code
sickchill/oldbeard/browser.py
Outdated
# if path == "/": | ||
# for name, share in settings.WINDOWS_SHARES.items(): | ||
# entries.append({"name": name, "path": "\\\\{server}\{path}".format(server=share["server"], path=share["path"])}) |
Check notice
Code scanning / CodeQL
Commented-out code
shares, count, unknown2 = win32net.NetShareEnum(f"\\\\{server['name']}", 0) | ||
for share in shares: | ||
network_shares.append(f"\\\\{server['name']}\\{share['netname']}") | ||
except Exception: |
Check notice
Code scanning / CodeQL
Empty except
|
||
return json.dumps(foldersAtPath(path, True, includeFiles, fileTypes)) | ||
path = Path(path) | ||
if not path.exists(): |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression
""" | ||
|
||
# Resolve symlinks and return a sane path | ||
path = path.resolve() |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression
path = path.resolve() | ||
|
||
# Go up in the path to the containing dir if this is a file | ||
while not path.is_dir(): |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression
0b2e6a7
to
75e7345
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 17
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
sickchill/gui/slick/js/core.min.js
is excluded by:!**/*.min.js
Files selected for processing (23)
- pyproject.toml (1 hunks)
- sickchill/gui/slick/js/browser.js (6 hunks)
- sickchill/gui/slick/js/core.js (9 hunks)
- sickchill/gui/slick/js/imageSelector.js (1 hunks)
- sickchill/gui/slick/js/rootDirs.js (2 hunks)
- sickchill/gui/slick/views/config_backuprestore.mako (2 hunks)
- sickchill/gui/slick/views/displayShow.mako (1 hunks)
- sickchill/init_helpers.py (2 hunks)
- sickchill/oldbeard/browser.py (2 hunks)
- sickchill/oldbeard/helpers.py (2 hunks)
- sickchill/oldbeard/notifiers/telegram.py (1 hunks)
- sickchill/oldbeard/nzbSplitter.py (1 hunks)
- sickchill/oldbeard/nzbget.py (1 hunks)
- sickchill/oldbeard/providers/btn.py (1 hunks)
- sickchill/tv.py (1 hunks)
- sickchill/update_manager/pip.py (2 hunks)
- sickchill/update_manager/runner.py (2 hunks)
- sickchill/views/api/webapi.py (1 hunks)
- sickchill/views/browser.py (2 hunks)
- sickchill/views/config/backup.py (3 hunks)
- sickchill/views/home.py (3 hunks)
- sickchill/views/index.py (1 hunks)
- tests/test_browser.py (1 hunks)
Files skipped from review due to trivial changes (1)
- sickchill/gui/slick/js/rootDirs.js
Additional comments: 23
sickchill/views/browser.py (4)
- 2-2: The import of
Path
frompathlib
is a positive change, promoting the use of modern path handling in Python.- 22-25: Converting the
path
string to aPath
object usingpathlib
is a good practice for more robust path handling. However, ensure that thepath
variable is sanitized before this conversion to prevent potential security issues related to path traversal.- 23-24: Checking if the path exists and defaulting to
settings.DATA_DIR
if it doesn't is a good fallback mechanism. However, ensure thatsettings.DATA_DIR
is a safe and expected default location for all use cases.- 36-38: The use of
folders_at_path
with parameters likeinclude_parent
,include_files
, andfile_types
provides flexibility in fetching directory contents. Ensure that the implementation offolders_at_path
properly handles these parameters to avoid any unintended behavior.sickchill/oldbeard/nzbSplitter.py (1)
- 125-125: The change from
if url_data is None:
toif not url_data:
broadens the check to include not onlyNone
but also other falsy values such as empty strings, which is a good practice for robustness. Just ensure that this broader check aligns with the expected input types forurl_data
.sickchill/gui/slick/js/imageSelector.js (1)
- 48-48: Correcting the syntax for passing the
imageType
parameter by explicitly specifying the key is a good practice for clarity and ensuring the parameter is correctly recognized by the server-side logic. This change likely addresses a bug and improves the reliability of thegetJSON
request.sickchill/oldbeard/nzbget.py (1)
- 119-119: The update from
data is None
tonot data
broadens the check to include not onlyNone
but also other falsy values such as empty strings, which is a good practice for robustness. Ensure that this broader check aligns with the expected input types fordata
.pyproject.toml (1)
- 291-291: Updating the
cinemagoer
dependency to a Git repository reference provides flexibility and access to specific changes not available in released versions. Ensure that the project is prepared to handle potential risks associated with this approach, such as breaking changes or instability, possibly through enhanced testing or continuous integration practices.sickchill/oldbeard/providers/btn.py (1)
- 45-45: The change from
if data is None
toif not data
broadens the condition to catch more falsy values, potentially improving the robustness of the authentication check. Consider adding a comment to clarify the intent behind this broadened condition.sickchill/gui/slick/js/browser.js (5)
- 47-47: Renaming
firstValue
tocurrentBrowserPath
improves code readability by making the variable's purpose clearer.- 69-72: Refactoring the logic for handling file types and permissions enhances code maintainability by simplifying complex conditions.
- 122-126: Adjusting the dialog positioning and sizing calculations improves the user interface experience by ensuring the dialog is appropriately sized and positioned.
- 137-137: Updating the callback parameter order in the button click handler may affect related functionality. Verify that all callback usages have been updated accordingly.
- 197-199: Enhancing error handling for local storage permissions improves the robustness of the application by gracefully handling permission issues.
sickchill/update_manager/runner.py (1)
- 62-62: Renaming the
_backup
method tobackup_to_dir
improves code readability by making the method's purpose clearer. Ensure that all references to the old method name have been updated throughout the application.sickchill/oldbeard/helpers.py (2)
- 1102-1102: The return value of
backup_config_zip
has been changed fromTrue
toarchive
. This change affects how the function's result is handled by its callers. Ensure that all usages of this function have been updated to handle the new return type, which is now the path to the created archive instead of a boolean value indicating success.- 1259-1259: The return value in exception cases for
getURL
was changed fromNone
to an empty string""
. This adjustment impacts how error conditions are interpreted by the function's consumers. It's crucial to verify that this change does not adversely affect the error handling logic in parts of the codebase that rely ongetURL
.sickchill/gui/slick/js/core.js (6)
- 383-391: The backup functionality uses
$.post
to send a backup request. It's important to ensure that the backup directory path is properly validated on the server side to prevent directory traversal attacks.- 396-398: Similar to the backup functionality, the restore functionality also uses
$.post
. Ensure that the file path for the backup file is securely validated on the server side to prevent the execution of arbitrary files.- 1386-1393: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1389-1402]
The
fillAbdExamples
function dynamically updates the UI based on user input. Ensure that any data dynamically inserted into the DOM is properly escaped to prevent XSS vulnerabilities.Verification successful
The
fillAbdExamples
function uses jQuery's.text()
method to insert data into the DOM, which inherently escapes HTML, thus preventing XSS vulnerabilities by treating the inserted data as plain text. This approach ensures that any data dynamically inserted into the DOM is properly escaped, addressing the concern raised in the review comment.* 1436-1443: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1439-1452]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for proper escaping of dynamically inserted data in the DOM. grep -R "fillAbdExamples" --include=*.jsLength of output: 260
Script:
#!/bin/bash # Extract the implementation of fillAbdExamples to review how it handles data before inserting into the DOM rg "function fillAbdExamples" -A 20 sickchill/gui/slick/js/core.jsLength of output: 887
The
fillSportsExamples
function similarly updates the UI dynamically. As withfillAbdExamples
, ensure that data inserted into the DOM is escaped to prevent XSS attacks.Verification successful
The
fillSportsExamples
function incore.js
uses the jQuery.text()
method to insert data into the DOM, which automatically escapes HTML, effectively mitigating the risk of XSS attacks. This addresses the concern raised in the review comment about ensuring that dynamically inserted data is properly escaped to prevent XSS attacks.* 2943-2945: When setting episode scene numbering, ensure that input values such as `forSeason`, `forEpisode`, `sceneSeason`, and `sceneEpisode` are validated both client-side and server-side to prevent SQL injection or other malicious data manipulation. * 2972-2973: Similar to the previous comment, when setting absolute scene numbering, ensure that `forAbsolute` and `sceneAbsolute` are properly validated to prevent injection attacks.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Confirm escaping of dynamically inserted data in the DOM for sports examples. grep -R "fillSportsExamples" --include=*.jsLength of output: 272
Script:
#!/bin/bash # Extract the implementation of `fillSportsExamples` from `core.js` grep -A 20 -B 2 "function fillSportsExamples()" sickchill/gui/slick/js/core.jsLength of output: 935
logger.debug(f"skipped {skipped} shows due to filters: genre: {genre}, limit: {limit}, kind: {kind}") | ||
if limit: | ||
upper_slice = min(page * limit + limit, len(shows_to_show)) | ||
lower_slice = min(page * limit, len(shows_to_show) - limit) | ||
|
||
number_of_pages = len(shows_to_show) // limit | ||
if len(shows_to_show) % limit: | ||
number_of_pages += 1 | ||
|
||
logger.info(f"Split home into {number_of_pages} pages") | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logging and pagination logic in the filter
method (lines 160-170) is functional but could be streamlined:
- The logging statement on line 160 might include sensitive information. Ensure that the logged data respects user privacy and security best practices.
- The manual calculation of pagination (lines 165-167) could be replaced with a more abstracted approach or a utility function to improve code maintainability and readability.
Review the logging statement for potential privacy concerns. Consider using a pagination library or utility function to handle pagination logic more cleanly.
if jwk_set: | ||
for key_dict in jwk_set["keys"]: | ||
public_key = jwt_algorithms_RSAAlgorithm.from_jwk(json.dumps(key_dict)) | ||
if jwt.decode(self.request.cookies["CF_Authorization"], key=public_key, audience=settings.CF_POLICY_AUD): | ||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
The examination of the getURL
function in helpers.py
indicates it includes a try block, suggesting some level of error handling, but the specifics of how errors are managed (e.g., returning None
or an empty dictionary) are not shown. Additionally, the snippet from index.py
does not display any exception handling around the jwt.decode
call, aligning with the initial suggestion to improve security by handling potential exceptions from this method. It's recommended to implement exception handling around jwt.decode
to manage invalid tokens or other issues securely.
Analysis chain
The addition of a check for jwk_set
before iterating over its keys is a good practice to avoid potential NoneType
iteration errors. However, it's important to ensure that the helpers.getURL
function, which retrieves jwk_set
, correctly handles all error scenarios and returns None
or an empty dictionary when appropriate. This is crucial for the robustness of the JWT authorization process.
Additionally, consider handling the case where jwt.decode
raises an exception due to an invalid token or other issues. Wrapping the jwt.decode
call in a try-except block and logging or handling the error appropriately can prevent unauthorized access and improve the security of the application.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that helpers.getURL function properly handles errors and returns None or an empty dictionary when appropriate.
grep -r "def getURL" .
# Verify that jwt.decode exceptions are handled elsewhere or consider adding exception handling.
grep -r "jwt.decode" .
Length of output: 252
Script:
#!/bin/bash
# Inspect the implementation of the getURL function for error handling
cat ./sickchill/oldbeard/helpers.py | grep -A 20 "def getURL("
# Check for exception handling around the jwt.decode usage in ./sickchill/views/index.py
grep -C 5 "jwt.decode" ./sickchill/views/index.py
Length of output: 1296
@@ -1513,7 +1513,7 @@ def run(self): | |||
os.mkdir(self.location) | |||
|
|||
logger.info(_("API: sc.backup backing up to {location}").format(location=self.location)) | |||
result = update_manager._backup(self.location) | |||
result = update_manager.backup_to_dir(self.location) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a line with only a tilde (~
) symbol on line 1516, which seems to be a formatting error or misplaced comment marker. This line should be removed as it serves no purpose and could potentially cause confusion or errors in documentation generation tools or other parsers that might process this code file.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
result = update_manager.backup_to_dir(self.location) | |
result = update_manager.backup_to_dir(self.location) |
The CMDSickChillBackup
class's run
method correctly initiates the backup process by calling update_manager.backup_to_dir(self.location)
. However, it's important to ensure that the self.location
variable is properly sanitized and validated before use to prevent any potential security issues, such as directory traversal attacks. Additionally, consider implementing more detailed error handling to provide users with specific reasons why a backup might have failed, if possible.
Consider adding input validation for self.location
to ensure it's a safe and valid directory path. Also, enhance error handling to catch and report specific backup failures.
os.mkdir(self.location) | ||
|
||
logger.info(_("API: sc.backup backing up to {location}").format(location=self.location)) | ||
result = update_manager._backup(self.location) | ||
result = update_manager.backup_to_dir(self.location) | ||
if result: | ||
logger.info(_("API: sc.backup successful!")) | ||
return _responds(RESULT_SUCCESS, msg="Backup successful") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1516]
Overall, the file implements a wide range of API commands for managing shows, episodes, and system settings within SickChill. While the specific review of the CMDSickChillBackup
class revealed a need for improved input validation and error handling, it's advisable to apply these principles across all classes to ensure the system's robustness and security. Additionally, consider implementing a unified approach to error handling and user feedback to enhance maintainability and user experience.
Apply consistent input validation and error handling across all API command classes. Consider creating utility functions or a base class to handle common tasks and reduce code duplication.
} | ||
|
||
logger.debug(f"{self.indexerid}: Obtained info from IMDb ->{self.imdb_info}") | ||
except (TypeError, ValueError, LookupError, OperationalError, imdb.IMDbError, NewConnectionError, MaxRetryError) as error: | ||
except ( | ||
TypeError, | ||
ValueError, | ||
LookupError, | ||
IOError, | ||
OperationalError, | ||
TimeoutError, | ||
imdb.IMDbDataAccessError, | ||
imdb.IMDbError, | ||
NewConnectionError, | ||
MaxRetryError, | ||
) as error: | ||
logger.info(f"Could not get IMDB info: see debug logs for details") | ||
logger.debug(f"IMDB traceback: {error}") | ||
except (SyntaxError, KeyError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The file imports a significant number of modules and packages, which is expected given its functionality. However, ensure that all imported modules are used within the file to avoid unnecessary imports, which can lead to increased memory usage and slower startup times.
Review the list of imports to remove any that are not used within the file.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Given the complexity and size of the TVShow
and TVEpisode
classes, consider breaking down this file into smaller modules or classes, each focusing on a specific aspect of the functionality (e.g., file handling, metadata management, episode renaming). This approach can improve maintainability, readability, and testability of the code.
Refactor the code to separate concerns into different modules or classes, enhancing modularity and maintainability.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The methods load_from_db
, load_from_indexer
, and rename
in both TVShow
and TVEpisode
classes are quite lengthy and perform multiple tasks. Consider refactoring these methods to break down their functionality into smaller, more focused methods. This can enhance readability, make the code easier to maintain, and simplify unit testing.
Refactor lengthy methods by extracting distinct functionalities into separate, smaller methods.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Ensure consistent and detailed logging throughout the file, especially in error handling blocks. Use appropriate logging levels (e.g., debug
, info
, warning
, error
) to differentiate between normal operation messages and error conditions. This can greatly aid in troubleshooting and understanding the code's behavior in production.
Review and enhance logging statements, particularly in error handling sections, to provide more context and detail.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The current design of the TVShow
and TVEpisode
classes could benefit from a more rigorous application of SOLID design principles. For instance, consider separating concerns more clearly to adhere to the Single Responsibility Principle and exploring dependency injection to reduce tight coupling between components.
Evaluate the code's architecture and consider refactoring to better adhere to SOLID design principles, enhancing modularity, flexibility, and testability.
|
||
try: | ||
return response if response_type in ("response", None) else response.json() if response_type == "json" else getattr(response, response_type, response) | ||
except ValueError: | ||
logger.debug(_("Requested a json response but response was not json, check the url: {url}").format(url=url)) | ||
return None | ||
return "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getURL
function's handling of the response type could lead to unexpected behavior if the response type requested is not supported by the requests.Response
object. It's recommended to explicitly handle unsupported response types by either logging a warning or raising an exception to prevent silent failures or ambiguous return values.
Consider adding an else clause to handle unsupported response types explicitly:
try:
return response if response_type in ("response", None) else response.json() if response_type == "json" else getattr(response, response_type, response)
+ except ValueError:
+ logger.debug(_("Requested a json response but response was not json, check the url: {url}").format(url=url))
+ except AttributeError:
+ logger.warning(_("Unsupported response type requested: {response_type}").format(response_type=response_type))
+ return None
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
return "" | |
return "" |
sickchill/gui/slick/js/core.js
Outdated
@@ -53,7 +53,7 @@ function notifyModal(message) { | |||
} | |||
|
|||
function addSiteMessage(level = 'danger', tag = '', message = '') { | |||
$.post(scRoot + '/ui/set_site_message', {level, tag, message}, siteMessages => { | |||
$.post(scRoot + '/ui/set_site_message', {level: level, tag: tag, message: message}, siteMessages => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
The addSiteMessage
function directly incorporates the level
, tag
, and message
parameters into the POST request without explicit client-side sanitization or encoding within the provided JavaScript context. While this does not automatically imply a security vulnerability, it raises concerns regarding potential Cross-Site Scripting (XSS) risks if these inputs are not adequately sanitized or escaped on the server side before being rendered in the application's UI.
Best practices in web development strongly recommend sanitizing and encoding user inputs on both the client and server sides to mitigate XSS risks effectively. This dual-layer approach helps ensure that malicious scripts are not executed in the users' browsers.
Given the information available, it is advisable to review the server-side handling of these parameters to ensure they are properly sanitized or escaped. Additionally, implementing client-side sanitization or encoding as a precautionary measure would enhance the application's security posture.
Analysis chain
The addSiteMessage
function uses a POST request to set a site message. Ensure that any user input passed to this function is properly sanitized to prevent XSS attacks.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that user input is sanitized before being used in the POST request.
grep -R "addSiteMessage" --include=*.js
Length of output: 90018
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- .github/workflows/check_release.yml (1 hunks)
- .github/workflows/pythonpackage.yml (1 hunks)
- .github/workflows/release.yml (1 hunks)
- .github/workflows/upgrade_dependencies.yml (1 hunks)
Additional comments: 8
.github/workflows/check_release.yml (2)
- 31-33: The change to install Poetry using
pipx
is a good practice as it isolates Poetry in its own environment, reducing potential conflicts with other Python packages.- 33-36: Using
actions/setup-python@v5
with a specific Python version (3.11
) and caching mechanism for Poetry is a solid approach to ensure consistency and faster builds by reusing the cache..github/workflows/upgrade_dependencies.yml (2)
- 24-26: Renaming the step to "Install poetry" and using
pipx
for the installation is a clear and isolated approach, enhancing the workflow's maintainability.- 26-29: Updating to
actions/setup-python@v5
and adding caching for Poetry dependencies are beneficial for build performance and consistency across runs..github/workflows/release.yml (2)
- 38-40: Switching to
pipx
for Poetry installation in the release workflow is a good practice for isolating the tool and avoiding conflicts.- 40-43: The update to
actions/setup-python@v5
for Python setup, specifying version3.11
, and enabling caching for Poetry aligns with best practices for efficient and consistent workflow runs..github/workflows/pythonpackage.yml (2)
- 106-108: Changing the step name to "Install poetry" and adopting
pipx
for Poetry installation is a clear and effective approach, enhancing isolation and reducing potential conflicts.- 108-111: The update to
actions/setup-python@v5
, specifying the Python version from the matrix, and implementing caching for Poetry are beneficial for build performance and consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
sickchill/gui/slick/js/core.min.js
is excluded by:!**/*.min.js
Files selected for processing (27)
- .github/workflows/check_release.yml (1 hunks)
- .github/workflows/pythonpackage.yml (1 hunks)
- .github/workflows/release.yml (1 hunks)
- .github/workflows/upgrade_dependencies.yml (1 hunks)
- pyproject.toml (1 hunks)
- sickchill/gui/slick/js/browser.js (6 hunks)
- sickchill/gui/slick/js/core.js (9 hunks)
- sickchill/gui/slick/js/imageSelector.js (1 hunks)
- sickchill/gui/slick/js/rootDirs.js (2 hunks)
- sickchill/gui/slick/views/config_backuprestore.mako (2 hunks)
- sickchill/gui/slick/views/displayShow.mako (1 hunks)
- sickchill/init_helpers.py (2 hunks)
- sickchill/oldbeard/browser.py (2 hunks)
- sickchill/oldbeard/helpers.py (2 hunks)
- sickchill/oldbeard/notifiers/telegram.py (1 hunks)
- sickchill/oldbeard/nzbSplitter.py (1 hunks)
- sickchill/oldbeard/nzbget.py (1 hunks)
- sickchill/oldbeard/providers/btn.py (1 hunks)
- sickchill/tv.py (1 hunks)
- sickchill/update_manager/pip.py (2 hunks)
- sickchill/update_manager/runner.py (2 hunks)
- sickchill/views/api/webapi.py (1 hunks)
- sickchill/views/browser.py (2 hunks)
- sickchill/views/config/backup.py (3 hunks)
- sickchill/views/home.py (3 hunks)
- sickchill/views/index.py (1 hunks)
- tests/test_browser.py (1 hunks)
Files skipped from review as they are similar to previous changes (25)
- .github/workflows/check_release.yml
- .github/workflows/pythonpackage.yml
- .github/workflows/release.yml
- .github/workflows/upgrade_dependencies.yml
- pyproject.toml
- sickchill/gui/slick/js/browser.js
- sickchill/gui/slick/js/core.js
- sickchill/gui/slick/js/imageSelector.js
- sickchill/gui/slick/js/rootDirs.js
- sickchill/gui/slick/views/config_backuprestore.mako
- sickchill/gui/slick/views/displayShow.mako
- sickchill/init_helpers.py
- sickchill/oldbeard/browser.py
- sickchill/oldbeard/helpers.py
- sickchill/oldbeard/notifiers/telegram.py
- sickchill/oldbeard/nzbSplitter.py
- sickchill/oldbeard/nzbget.py
- sickchill/oldbeard/providers/btn.py
- sickchill/tv.py
- sickchill/update_manager/pip.py
- sickchill/update_manager/runner.py
- sickchill/views/api/webapi.py
- sickchill/views/browser.py
- sickchill/views/config/backup.py
- sickchill/views/index.py
Additional comments: 6
tests/test_browser.py (6)
- 5-5: The import of
Path
frompathlib
is a positive change, promoting the use of modern Python practices for file system paths handling, which enhances readability and cross-platform compatibility.- 16-16: The initialization of
self.here
usingPath(__file__)
is a good practice, leveragingpathlib
for path manipulation. This change aligns with modern Python standards and improves code readability and maintainability.- 23-23: Renaming
getWinDrives
toget_windows_drives
in the call withintest_get_win_drives
method follows Python's snake_case naming convention, improving code readability and consistency.- 31-31: The use of
self.here / "not_a_real_path"
demonstrates the advantage ofpathlib
for path manipulation, making the code more readable and Pythonic. This change is consistent with the overall objective of enhancing path handling in the backend.- 37-37: The re-call to
get_windows_drives
within the conditional block for Windows systems is consistent with the renaming and refactoring efforts. This ensures that the test logic aligns with the updated function names and practices.- 44-44: The test case
test_list = browser.folders_at_path(self.here, include_parent=True)
correctly demonstrates the use ofpathlib
for path manipulation and the inclusion of a parent directory in the test list. This is a good practice for testing directory listing functionalities.
tests/test_browser.py
Outdated
assert test_list[0]["currentPath"] == self.here | ||
|
||
test_list = browser.foldersAtPath("") | ||
test_list = browser.folders_at_path(Path("")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Path("")
to represent the current directory is a clear and concise way to utilize pathlib
, though it's worth noting that Path.cwd()
might be a more explicit choice for representing the current working directory. However, this usage is acceptable as it aligns with the context of testing path handling functionality.
Consider using Path.cwd()
when the intention is to explicitly represent the current working directory, as it enhances code clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- sickchill/gui/slick/js/browser.js (6 hunks)
- sickchill/gui/slick/js/core.js (9 hunks)
- sickchill/gui/slick/js/imageSelector.js (1 hunks)
- sickchill/gui/slick/js/rootDirs.js (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- sickchill/gui/slick/js/browser.js
- sickchill/gui/slick/js/core.js
- sickchill/gui/slick/js/imageSelector.js
- sickchill/gui/slick/js/rootDirs.js
…ck the diff and keep fixing fix: use named parameters when posting from javascript, fixes site messages, testEmail, testNaming variations, and isNamingValid clean up browser.folders_at_path increase performance of home Signed-off-by: miigotu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
sickchill/gui/slick/js/core.min.js
is excluded by:!**/*.min.js
Files selected for processing (22)
- pyproject.toml (1 hunks)
- sickchill/gui/slick/js/browser.js (6 hunks)
- sickchill/gui/slick/js/core.js (5 hunks)
- sickchill/gui/slick/js/rootDirs.js (2 hunks)
- sickchill/gui/slick/views/config_backuprestore.mako (2 hunks)
- sickchill/gui/slick/views/displayShow.mako (1 hunks)
- sickchill/init_helpers.py (2 hunks)
- sickchill/oldbeard/browser.py (2 hunks)
- sickchill/oldbeard/helpers.py (2 hunks)
- sickchill/oldbeard/notifiers/telegram.py (1 hunks)
- sickchill/oldbeard/nzbSplitter.py (1 hunks)
- sickchill/oldbeard/nzbget.py (1 hunks)
- sickchill/oldbeard/providers/btn.py (1 hunks)
- sickchill/tv.py (1 hunks)
- sickchill/update_manager/pip.py (2 hunks)
- sickchill/update_manager/runner.py (2 hunks)
- sickchill/views/api/webapi.py (1 hunks)
- sickchill/views/browser.py (2 hunks)
- sickchill/views/config/backup.py (3 hunks)
- sickchill/views/home.py (3 hunks)
- sickchill/views/index.py (1 hunks)
- tests/test_browser.py (1 hunks)
Files skipped from review as they are similar to previous changes (20)
- pyproject.toml
- sickchill/gui/slick/js/browser.js
- sickchill/gui/slick/js/core.js
- sickchill/gui/slick/js/rootDirs.js
- sickchill/gui/slick/views/config_backuprestore.mako
- sickchill/gui/slick/views/displayShow.mako
- sickchill/init_helpers.py
- sickchill/oldbeard/browser.py
- sickchill/oldbeard/helpers.py
- sickchill/oldbeard/notifiers/telegram.py
- sickchill/oldbeard/nzbSplitter.py
- sickchill/oldbeard/nzbget.py
- sickchill/oldbeard/providers/btn.py
- sickchill/tv.py
- sickchill/update_manager/pip.py
- sickchill/update_manager/runner.py
- sickchill/views/api/webapi.py
- sickchill/views/browser.py
- sickchill/views/config/backup.py
- sickchill/views/index.py
Additional comments: 3
tests/test_browser.py (3)
- 16-16: The initialization of
self.here
usingPath(__file__)
is a good practice for leveragingpathlib
for path handling, enhancing readability and maintainability.- 23-23: The method call
browser.get_windows_drives()
correctly reflects the updated method name to follow the snake_case naming convention. This change improves code readability and consistency with Python standards.- 31-31: Using
self.here / "not_a_real_path"
demonstrates the effective use ofpathlib
for path manipulation. This approach is more readable and idiomatic compared to traditional string concatenation oros.path
functions.
tests/test_browser.py
Outdated
test_list = browser.folders_at_path(Path("")) | ||
if os.name == "nt": | ||
assert test_list[0]["currentPath"] == "Root" | ||
drives = browser.getWinDrives() | ||
drives = browser.get_windows_drives() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test case for folders_at_path(Path(""))
and its subsequent assertions are well-structured. However, consider the previous suggestion about using Path.cwd()
for clarity when the intention is to represent the current working directory. This is not a mandatory change but could enhance code clarity in tests that specifically aim to represent the current directory.
- test_list = browser.folders_at_path(Path(""))
+ test_list = browser.folders_at_path(Path.cwd())
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
test_list = browser.folders_at_path(Path("")) | |
if os.name == "nt": | |
assert test_list[0]["currentPath"] == "Root" | |
drives = browser.getWinDrives() | |
drives = browser.get_windows_drives() | |
test_list = browser.folders_at_path(Path.cwd()) | |
if os.name == "nt": | |
assert test_list[0]["currentPath"] == "Root" | |
drives = browser.get_windows_drives() |
assert len(drives) == len(test_list[1:]) | ||
for item in test_list[1:]: | ||
assert item["path"].strip(":\\") in drives | ||
else: | ||
assert test_list[0]["currentPath"] == "/" | ||
|
||
test_list = browser.foldersAtPath(os.path.join(self.here), includeParent=True) | ||
test_list = browser.folders_at_path(self.here, include_parent=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assertion assert test_list[1]["path"] == os.path.dirname(self.here)
mixes pathlib
and os.path
functionalities. As previously commented, for consistency and to fully leverage pathlib
, consider converting self.here
to a string when comparing paths or use pathlib
's methods for directory manipulation. This change would align with the overall direction of the codebase towards using pathlib
.
- assert test_list[1]["path"] == os.path.dirname(self.here)
+ assert test_list[1]["path"] == str(self.here.parent)
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
test_list = browser.folders_at_path(self.here, include_parent=True) | |
test_list = browser.folders_at_path(self.here, include_parent=True) |
sickchill/views/home.py
Outdated
return t.render( | ||
title=_("Home"), | ||
header=_("Show List"), | ||
topmenu="home", | ||
sorted_show_lists=[["Shows", shows], ["Anime", anime]], | ||
show_stat=stats[0], | ||
max_download_count=stats[1], | ||
controller="home", | ||
action="index", | ||
selected_root=selected_root or "-1", | ||
) | ||
|
||
def filter(self): | ||
t = PageTemplate(rh=self, filename="home.mako") | ||
|
||
sorted_show_lists = [ | ||
["Shows", sorted(shows, key=lambda mbr: attrgetter("sort_name")(mbr))], | ||
["Anime", sorted(anime, key=lambda mbr: attrgetter("sort_name")(mbr))], | ||
] | ||
selected_root = self.get_body_argument("root", None) | ||
page = try_int(self.get_argument("p", default="0")) | ||
limit = try_int(self.get_argument("limit", default=None)) | ||
kind = self.get_argument("type", "all") | ||
genre = self.get_argument("genre", "") | ||
if kind not in ("all", "series", "anime"): | ||
kind = "all" | ||
|
||
if selected_root and settings.ROOT_DIRS: | ||
backend_pieces = settings.ROOT_DIRS.split("|") | ||
backend_dirs = backend_pieces[1:] | ||
try: | ||
assert selected_root != "-1" | ||
selected_root_dir = backend_dirs[int(selected_root)] | ||
if selected_root_dir[-1] not in ("/", "\\"): | ||
selected_root_dir += os.sep | ||
except (IndexError, ValueError, TypeError, AssertionError): | ||
selected_root_dir = "" | ||
else: | ||
shows = [] | ||
for show in settings.showList: | ||
# noinspection PyProtectedMember | ||
if selected_root_dir in show.get_location: | ||
shows.append(show) | ||
selected_root_dir = "" | ||
|
||
shows_to_show = [] | ||
skipped = 0 | ||
for show in settings.showList: | ||
if selected_root_dir and selected_root_dir not in show.get_location: | ||
continue | ||
|
||
if kind == "anime" and not show.is_anime: | ||
skipped += 1 | ||
continue | ||
|
||
if kind == "series" and show.is_anime: | ||
skipped += 1 | ||
continue | ||
|
||
sorted_show_lists = [["Shows", sorted(shows, key=lambda mbr: attrgetter("sort_name")(mbr))]] | ||
if genre and genre.lower() not in show.genre: | ||
skipped += 1 | ||
continue | ||
|
||
shows_to_show.append(show) | ||
if limit and len(shows_to_show) == limit: | ||
break | ||
|
||
logger.debug(f"skipped {skipped} shows due to filters: genre: {genre}, limit: {limit}, kind: {kind}") | ||
if limit: | ||
upper_slice = min(page * limit + limit, len(shows_to_show)) | ||
lower_slice = min(page * limit, len(shows_to_show) - limit) | ||
|
||
number_of_pages = len(shows_to_show) // limit | ||
if len(shows_to_show) % limit: | ||
number_of_pages += 1 | ||
|
||
logger.info(f"Split home into {number_of_pages} pages") | ||
else: | ||
upper_slice = len(shows_to_show) - 1 | ||
lower_slice = 0 | ||
|
||
shows = [] | ||
anime = [] | ||
for show in shows_to_show[lower_slice:upper_slice]: | ||
if settings.ANIME_SPLIT_HOME and show.is_anime: | ||
anime.append(show) | ||
else: | ||
shows.append(show) | ||
|
||
stats = self.show_statistics() | ||
return t.render( | ||
title=_("Home"), | ||
header=_("Show List"), | ||
topmenu="home", | ||
sorted_show_lists=sorted_show_lists, | ||
sorted_show_lists=[["Shows", shows], ["Anime", anime]], | ||
show_stat=stats[0], | ||
max_download_count=stats[1], | ||
controller="home", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [78-112]
The logic for handling root directories and filtering shows based on the selected root directory seems to be duplicated in the index
method. Consider extracting this logic into a shared function to reduce code duplication and improve maintainability.
def _get_selected_root_dir(selected_root):
if selected_root and settings.ROOT_DIRS:
backend_pieces = settings.ROOT_DIRS.split("|")
backend_dirs = backend_pieces[1:]
try:
assert selected_root != "-1"
selected_root_dir = backend_dirs[int(selected_root)]
if selected_root_dir[-1] not in ("/", "\\"):
selected_root_dir += os.sep
except (IndexError, ValueError, TypeError, AssertionError):
selected_root_dir = ""
else:
selected_root_dir = ""
return selected_root_dir
Then, replace the duplicated logic in both index
and filter
methods with a call to _get_selected_root_dir(selected_root)
.
sickchill/views/home.py
Outdated
def filter(self): | ||
t = PageTemplate(rh=self, filename="home.mako") | ||
|
||
sorted_show_lists = [ | ||
["Shows", sorted(shows, key=lambda mbr: attrgetter("sort_name")(mbr))], | ||
["Anime", sorted(anime, key=lambda mbr: attrgetter("sort_name")(mbr))], | ||
] | ||
selected_root = self.get_body_argument("root", None) | ||
page = try_int(self.get_argument("p", default="0")) | ||
limit = try_int(self.get_argument("limit", default=None)) | ||
kind = self.get_argument("type", "all") | ||
genre = self.get_argument("genre", "") | ||
if kind not in ("all", "series", "anime"): | ||
kind = "all" | ||
|
||
if selected_root and settings.ROOT_DIRS: | ||
backend_pieces = settings.ROOT_DIRS.split("|") | ||
backend_dirs = backend_pieces[1:] | ||
try: | ||
assert selected_root != "-1" | ||
selected_root_dir = backend_dirs[int(selected_root)] | ||
if selected_root_dir[-1] not in ("/", "\\"): | ||
selected_root_dir += os.sep | ||
except (IndexError, ValueError, TypeError, AssertionError): | ||
selected_root_dir = "" | ||
else: | ||
shows = [] | ||
for show in settings.showList: | ||
# noinspection PyProtectedMember | ||
if selected_root_dir in show.get_location: | ||
shows.append(show) | ||
selected_root_dir = "" | ||
|
||
shows_to_show = [] | ||
skipped = 0 | ||
for show in settings.showList: | ||
if selected_root_dir and selected_root_dir not in show.get_location: | ||
continue | ||
|
||
if kind == "anime" and not show.is_anime: | ||
skipped += 1 | ||
continue | ||
|
||
if kind == "series" and show.is_anime: | ||
skipped += 1 | ||
continue | ||
|
||
sorted_show_lists = [["Shows", sorted(shows, key=lambda mbr: attrgetter("sort_name")(mbr))]] | ||
if genre and genre.lower() not in show.genre: | ||
skipped += 1 | ||
continue | ||
|
||
shows_to_show.append(show) | ||
if limit and len(shows_to_show) == limit: | ||
break | ||
|
||
logger.debug(f"skipped {skipped} shows due to filters: genre: {genre}, limit: {limit}, kind: {kind}") | ||
if limit: | ||
upper_slice = min(page * limit + limit, len(shows_to_show)) | ||
lower_slice = min(page * limit, len(shows_to_show) - limit) | ||
|
||
number_of_pages = len(shows_to_show) // limit | ||
if len(shows_to_show) % limit: | ||
number_of_pages += 1 | ||
|
||
logger.info(f"Split home into {number_of_pages} pages") | ||
else: | ||
upper_slice = len(shows_to_show) - 1 | ||
lower_slice = 0 | ||
|
||
shows = [] | ||
anime = [] | ||
for show in shows_to_show[lower_slice:upper_slice]: | ||
if settings.ANIME_SPLIT_HOME and show.is_anime: | ||
anime.append(show) | ||
else: | ||
shows.append(show) | ||
|
||
stats = self.show_statistics() | ||
return t.render( | ||
title=_("Home"), | ||
header=_("Show List"), | ||
topmenu="home", | ||
sorted_show_lists=sorted_show_lists, | ||
sorted_show_lists=[["Shows", shows], ["Anime", anime]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filter
method introduces pagination and filtering functionality for shows based on various criteria like type and genre. However, there are a few areas that could be improved for better performance and maintainability:
- The method of splitting
settings.ROOT_DIRS
and accessing its elements is repeated multiple times. Consider refactoring this into a separate function to reduce redundancy and improve code readability. - The use of
assert
for control flow (lines 129 and 133) is not recommended in production code as assertions can be globally disabled with the-O
and-OO
flags, leading to unexpected behavior. It's better to use explicit error handling. - The calculation for pagination (lines 162-167) is manually implemented. Consider using a library or a utility function to handle pagination logic to reduce complexity and potential errors.
- The logging statement on line 160 could potentially leak sensitive information about the shows being skipped. Ensure that logging does not expose any personal or sensitive data.
Consider refactoring the repeated logic for handling settings.ROOT_DIRS
into a separate function. Replace assert
statements with explicit error handling. Use a library or utility function for pagination logic. Review the logging statement to ensure it does not leak sensitive information.
sickchill/views/home.py
Outdated
["Shows", sorted(shows, key=lambda mbr: attrgetter("sort_name")(mbr))], | ||
["Anime", sorted(anime, key=lambda mbr: attrgetter("sort_name")(mbr))], | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sorting logic for shows and anime (lines 992-994) directly uses attrgetter
within a lambda function. While this works, it introduces unnecessary complexity into the sorting process:
- Consider using a key function directly without the lambda for improved readability.
- Ensure that the sorting criteria are well-documented, as the current implementation assumes knowledge of the
sort_name
attribute's significance.
# Suggestion to refactor the sorting logic to use a key function directly for improved code clarity
sorted_shows = sorted(shows, key=attrgetter("sort_name"))
sorted_anime = sorted(anime, key=attrgetter("sort_name"))
Add comments to document the sorting criteria and the significance of the sort_name
attribute.
…indows Signed-off-by: miigotu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- sickchill/oldbeard/browser.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- sickchill/oldbeard/browser.py
feat: check if sickchill is trying to stop while running through karge loops and break out so it can stop sooner feat: pre-sort show_list when loading from the database and after adding a show, so we dont need to do it between a web request and web response chore: remove unused code for is_anime_in_show_list, update_anime_support, and settings.ANIMESUPPORT. Signed-off-by: miigotu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 11
Configuration used: CodeRabbit UI
Files selected for processing (43)
- SickChill.py (2 hunks)
- frontend/movies/views.py (1 hunks)
- frontend/shows/views.py (1 hunks)
- sickchill/gui/slick/views/manage.mako (2 hunks)
- sickchill/oldbeard/dailysearcher.py (1 hunks)
- sickchill/oldbeard/helpers.py (7 hunks)
- sickchill/oldbeard/name_cache.py (2 hunks)
- sickchill/oldbeard/notifiers/trakt.py (1 hunks)
- sickchill/oldbeard/postProcessor.py (1 hunks)
- sickchill/oldbeard/scene_exceptions.py (3 hunks)
- sickchill/oldbeard/scene_numbering.py (3 hunks)
- sickchill/oldbeard/search.py (1 hunks)
- sickchill/oldbeard/searchBacklog.py (1 hunks)
- sickchill/oldbeard/show_queue.py (4 hunks)
- sickchill/oldbeard/subtitles.py (1 hunks)
- sickchill/oldbeard/traktChecker.py (5 hunks)
- sickchill/oldbeard/tvcache.py (2 hunks)
- sickchill/providers/torrent/TorrentProvider.py (1 hunks)
- sickchill/settings.py (3 hunks)
- sickchill/show/ComingEpisodes.py (1 hunks)
- sickchill/show/Show.py (2 hunks)
- sickchill/show/indexers/handler.py (1 hunks)
- sickchill/show/recommendations/favorites.py (1 hunks)
- sickchill/show/recommendations/recommended.py (1 hunks)
- sickchill/show_updater.py (1 hunks)
- sickchill/start.py (2 hunks)
- sickchill/system/Restart.py (1 hunks)
- sickchill/system/Shutdown.py (1 hunks)
- sickchill/tv.py (5 hunks)
- sickchill/views/api/webapi.py (20 hunks)
- sickchill/views/home.py (19 hunks)
- sickchill/views/imageSelector.py (1 hunks)
- sickchill/views/index.py (4 hunks)
- sickchill/views/manage/add_shows.py (4 hunks)
- sickchill/views/manage/index.py (6 hunks)
- tests/conftest.py (4 hunks)
- tests/sickchill_tests/show/test_show.py (2 hunks)
- tests/test_helpers.py (3 hunks)
- tests/test_pp.py (1 hunks)
- tests/test_search.py (1 hunks)
- tests/test_snatch.py (1 hunks)
- tests/test_tv.py (3 hunks)
- tests/test_xem.py (2 hunks)
Files not reviewed due to errors (1)
- sickchill/start.py (Error: unable to parse review)
Files skipped from review due to trivial changes (4)
- frontend/shows/views.py
- sickchill/oldbeard/search.py
- sickchill/show/recommendations/favorites.py
- tests/test_helpers.py
Files skipped from review as they are similar to previous changes (4)
- sickchill/oldbeard/helpers.py
- sickchill/tv.py
- sickchill/views/api/webapi.py
- sickchill/views/index.py
Additional comments: 72
sickchill/system/Shutdown.py (1)
- 15-15: The addition of
settings.stopping = True
after triggering the shutdown event is a good practice for signaling the system's stopping state. Ensure that this flag is checked in other parts of the application to gracefully halt operations or cleanup resources.sickchill/system/Restart.py (1)
- 16-16: The addition of
settings.restarting = True
after triggering the restart event is a good practice for signaling the system's restarting state. Ensure that this flag is checked in other parts of the application to manage the restart process effectively.sickchill/show/recommendations/recommended.py (1)
- 42-42: Renaming
settings.showList
tosettings.show_list
aligns with Python's naming conventions and improves code readability. Ensure that this new variable name is consistently used across the entire codebase.tests/test_xem.py (2)
- 18-24: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [21-30]
Renaming
showList
toshow_list
in theload_shows_from_db
method aligns with Python's naming conventions and improves code readability. Ensure that this new variable name is consistently used across the entire codebase.
- 37-45: Renaming
showList
toshow_list
in theload_from_db
method aligns with Python's naming conventions and improves code readability. Ensure that this new variable name is consistently used across the entire codebase.sickchill/views/imageSelector.py (1)
- 25-25: Renaming
showList
toshow_list
in theShow.find
function call aligns with Python's naming conventions and improves code readability. Ensure that this new variable name is consistently used across the entire codebase.sickchill/oldbeard/name_cache.py (2)
- 43-43: Renaming
showList
toshow_list
in theget_id_from_name
function aligns with Python's naming conventions and improves code readability. Ensure that this new variable name is consistently used across the entire codebase.- 80-83: Adding checks for
settings.stopping
andsettings.restarting
in thebuild_name_cache
function is a good practice for gracefully handling shutdown and restart processes. Ensure that these checks are effectively used throughout the codebase to manage system states appropriately.tests/test_tv.py (3)
- 86-86: Renaming
showList
toshow_list
in thesetUp
method ofTVEpisodeTests
aligns with Python's naming conventions and improves code readability. Ensure that this new variable name is consistently used across the entire codebase.- 110-110: Renaming
showList
toshow_list
in thesetUp
method ofTVTests
aligns with Python's naming conventions and improves code readability. Ensure that this new variable name is consistently used across the entire codebase.- 127-127: Assigning
show_list
directly in thetest_get_episode
method ofTVTests
aligns with Python's naming conventions and improves code readability. Ensure that this new variable name is consistently used across the entire codebase.tests/sickchill_tests/show/test_show.py (2)
- 25-25: Renaming
showList
toshow_list
in thetest_find
function aligns with Python's naming conventions and improves code readability. Ensure that this new variable name is consistently used across the entire codebase.- 71-76: Renaming
showList
toshow_list
and using it in thetest_validate_indexer_id
function aligns with Python's naming conventions and improves code readability. Ensure that this new variable name is consistently used across the entire codebase.sickchill/providers/torrent/TorrentProvider.py (1)
- 31-31: Renaming
showList
toshow_list
in thefind_propers
method aligns with Python's naming conventions and improves code readability. Ensure that this new variable name is consistently used across the entire codebase.sickchill/oldbeard/dailysearcher.py (1)
- 50-50: Renaming
showList
toshow_list
in therun
method aligns with Python's naming conventions and improves code readability. Ensure that this new variable name is consistently used across the entire codebase.tests/test_snatch.py (1)
- 119-119: The update from
showList
toshow_list
improves naming consistency and adheres to Python's naming conventions. Good job on enhancing code readability and maintainability.sickchill/show_updater.py (1)
- 51-53: The addition of checks for
settings.stopping
orsettings.restarting
within the show update loop is a smart way to enhance application responsiveness to state changes. This ensures unnecessary processing is avoided during shutdown or restart operations.tests/test_search.py (1)
- 48-48: Updating
showList
toshow_list
in thesettings
object enhances naming consistency and adheres to Python's naming conventions. This is a positive change for code readability and maintainability.tests/test_pp.py (1)
- 55-55: The change from
settings.showList
tosettings.show_list
in thetest_process
function is consistent with efforts to improve naming conventions across the codebase. This enhances code readability and maintainability.sickchill/oldbeard/searchBacklog.py (1)
- 58-58: Updating the variable assignment from
settings.showList
tosettings.show_list
insearchBacklog.py
aligns with the effort to improve naming conventions across the application. This is a positive step towards enhancing code readability and maintainability.sickchill/show/ComingEpisodes.py (1)
- 78-78: The update from
showList
toshow_list
within the loop inComingEpisodes.py
contributes to the ongoing effort to improve naming conventions across the application. This enhances code readability and maintainability.sickchill/oldbeard/notifiers/trakt.py (1)
- 119-125: The renaming of
showList
toshow_list
within thetrakt_show_data_generate
method improves consistency with Python's naming conventions. Good job on enhancing code readability.sickchill/show/Show.py (2)
- 110-110: Updating references from
settings.showList
tosettings.show_list
in theoverall_stats
method aligns with Python's naming conventions and improves code readability.- 207-207: The change in the
validate_indexer_id
method fromsettings.showList
tosettings.show_list
is consistent with the effort to standardize variable naming across the codebase.sickchill/show/indexers/handler.py (1)
- 123-123: The update from
settings.showList
tosettings.show_list
in thesearch_indexers_for_series_id
method is a good practice for maintaining consistency with Python's naming conventions.tests/conftest.py (6)
- 85-85: Initializing
settings.show_list
as an empty list at the start of the test setup is consistent with the updated naming convention and ensures that the test environment is correctly initialized.- 161-161: Resetting
settings.show_list
to an empty list in thesetUp
method ofSickChillTestDBCase
is a good practice to ensure a clean state for each test case.- 167-167: Resetting
settings.show_list
to an empty list in thetearDown
method ofSickChillTestDBCase
ensures that the test environment is correctly cleaned up after each test case.- 183-183: Initializing
settings.show_list
in thesetUp
method ofSickChillTestPostProcessorCase
is consistent with the updated naming convention and ensures that the test environment is correctly initialized for post-processing tests.- 206-206: Adding the show to
settings.show_list
in thesetUp
method ofSickChillTestPostProcessorCase
is necessary for the test setup and aligns with the updated naming convention.- 209-209: Resetting
settings.show_list
to an empty list in thetearDown
method ofSickChillTestPostProcessorCase
ensures that the test environment is correctly cleaned up after each post-processing test case.sickchill/oldbeard/scene_exceptions.py (3)
- 66-66: The change from
settings.showList
tosettings.show_list
improves consistency with Python naming conventions. This is a positive change that aligns with best practices for variable naming.- 107-107: Similar to the previous comment, updating the variable name here also improves consistency and readability.
- 287-290: Adding a conditional check for
settings.stopping
andsettings.restarting
within the loop is a good practice. It allows the function to exit early if the application is in the process of stopping or restarting, potentially avoiding unnecessary work or errors during shutdown.SickChill.py (2)
- 14-14: Adding the
attrgetter
import from theoperator
module is necessary for the new sorting functionality introduced later in the file. This is a correct and efficient way to sort objects based on attributes.- 247-270: The changes here include renaming
showList
toshow_list
for consistency with Python naming conventions, adding checks forsettings.stopping
andsettings.restarting
to exit the loop early during shutdown or restart, and handlingKeyboardInterrupt
to break out of the loop. Additionally, theshow_list
is now presorted based onsort_name
, which is a good performance optimization as it avoids sorting on every page load. These changes improve code readability, maintainability, and performance.sickchill/gui/slick/views/manage.mako (1)
- 82-82: Refactoring the iteration over
settings.showList
to usesettings.show_list
in the Mako template aligns with the changes made in the Python backend for consistency. This is a good practice as it ensures that the naming conventions are consistent across both the backend and frontend code, improving code readability and maintainability.sickchill/settings.py (2)
- 385-385: Renaming
showList
toshow_list
improves consistency with Python's naming conventions for variables. This change aligns with the PEP 8 style guide, which recommends lowercase with words separated by underscores as necessary to improve readability.- 418-419: The addition of
stopping
andrestarting
flags introduces new state management variables. It's crucial to ensure that these flags are properly managed throughout the application lifecycle to avoid any unintended side effects. Consider adding comments to describe their purpose and how they should be used within the application. Additionally, verify that their state is correctly updated in all relevant parts of the code to prevent any inconsistencies.Verification successful
The search results indicate that the
stopping
andrestarting
flags are being actively managed across the application. They are set toTrue
in appropriate scenarios (during shutdown and restart processes) and are checked in various parts of the application to modify behavior accordingly. This suggests that the flags are integrated into the application's lifecycle management as intended. However, there's no direct evidence of the flags being reset toFalse
, which might not be necessary if the application is expected to terminate or fully restart after these flags are set, as the application state would be re-initialized on the next start. Based on this analysis, the review comment appears to be validated by the codebase usage of these flags.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of `stopping` and `restarting` flags to ensure proper management. rg --type py 'stopping|restarting'Length of output: 1291
sickchill/oldbeard/tvcache.py (2)
- 346-346: The reference to
settings.showList
has been updated tosettings.show_list
for consistency. This change aligns with Python's naming conventions and improves code readability. However, ensure that all references toshowList
throughout the codebase have been updated toshow_list
to maintain consistency and avoid potential errors.Verification successful
The script did not find any occurrences of
settings.showList
in Python files, suggesting that the renaming tosettings.show_list
has been consistently applied across the codebase, at least within the scope of Python files. This supports the correctness of the change. However, it's important to ensure that all references, including those in configuration files, documentation, or other non-Python code, have also been updated to maintain consistency and avoid potential errors. Given the information available, the change appears to be correctly implemented within the Python files.* 451-451: Similar to the previous comment, the reference to `settings.showList` has been updated to `settings.show_list` in the `find_needed_episodes` function. This change is consistent with Python's naming conventions. As before, verify that all instances of `showList` have been updated across the entire codebase to avoid inconsistencies.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old `showList` to ensure consistency. rg --type py 'settings\.showList'Length of output: 35
Verification successful
The search for any remaining references to
settings.showList
in Python files did not produce any results, indicating that all instances have been updated tosettings.show_list
across the entire codebase. This change aligns with Python's naming conventions and the intent of the review comment.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Repeating the search for any remaining references to the old `showList` to ensure consistency. rg --type py 'settings\.showList'Length of output: 35
sickchill/oldbeard/subtitles.py (1)
- 439-439: The variable
showList
is mentioned in the AI-generated summary as being renamed toshow_list
, but the actual code change or usage is not directly visible in the provided diff. It's possible the change is related to how shows are accessed within theSubtitlesFinder
class, specifically in the line whereShow.find
is called. This might imply a broader change in how lists of shows are managed or named within the application. However, without a direct reference toshowList
orshow_list
in the provided code, it's challenging to assess the impact or correctness of this specific change.It would be beneficial to verify the consistency of this variable renaming across the entire codebase to ensure that all references to the old
showList
variable have been updated accordingly. This can help avoid potential runtime errors due to undefined variables.sickchill/oldbeard/scene_numbering.py (3)
- 35-35: The reference to
settings.show_list
has been updated fromsettings.showList
for consistency. This change aligns with Python's naming conventions and improves code readability. Ensure all instances whereshowList
was used are updated toshow_list
to maintain consistency across the application.- 89-89: Similar to the previous comment, the reference to
settings.show_list
here has been correctly updated for consistency. This change is part of the broader effort to standardize naming conventions within the SickChill application. As with the previous instance, it's important to verify that all references to the oldshowList
variable have been updated accordingly.- 212-212: The update to
settings.show_list
in this line is consistent with the changes made elsewhere in the file. This modification is part of the effort to improve code readability and maintainability by adhering to Python's naming conventions. It's crucial to ensure that this change is reflected across all files that interact with thesettings
module to avoid any potential issues.sickchill/views/manage/add_shows.py (4)
- 98-98: The change from
settings.showList
tosettings.show_list
in thesearchIndexersForShowName
method is consistent with the PR's objective to improve code readability and maintainability. However, ensure that all references toshow_list
throughout the application have been updated accordingly to prevent any AttributeError exceptions.- 179-179: Renaming
settings.showList
tosettings.show_list
in themassAddTable
method aligns with the goal of enhancing code consistency. It's crucial to verify that thesettings
object'sshow_list
attribute is correctly populated and used elsewhere in the application to maintain functionality.- 324-324: The update to
settings.show_list
in thepopularShows
method is appropriate for maintaining consistency in variable naming conventions. Double-check that the logic for filtering shows based on their IMDb ID remains intact and functions as expected after this change.- 425-425: The modification to use
settings.show_list
in theaddShowByID
method supports the PR's objective of codebase consistency and readability improvements. It's important to ensure that theShow.find
method correctly handles the updatedshow_list
attribute without affecting the application's ability to add shows by ID.sickchill/oldbeard/show_queue.py (1)
- 420-420: The use of
Show.find(settings.show_list, self.indexer_id)
to check if a show exists insettings.show_list
before deciding whether to raise an exception or proceed with adding the show is a good practice. However, it's important to ensure thatShow.find
is an efficient method, especially ifsettings.show_list
can become large. Consider optimizing the search operation if necessary, for example, by maintaining a more efficient data structure forshow_list
that allows quicker lookups.sickchill/oldbeard/traktChecker.py (5)
- 292-295: The addition of conditions to break the loop based on
settings.stopping
orsettings.restarting
is a good practice for gracefully handling application shutdown or restart. However, ensure that these settings are updated in a thread-safe manner elsewhere in the application to prevent race conditions.- 316-319: Similar to the previous comment, adding conditions to break the loop based on
settings.stopping
orsettings.restarting
in the_remove_watched_shows_from_sickchill
method is a good practice. Again, ensure thread safety when updating these settings.- 360-360: The change from
settings.showList
tosettings.show_list
in theShow.find
function call aligns with Python's naming conventions and improves code readability. Ensure that theShow.find
method is updated accordingly to accept the new parameter name.- 388-388: The same improvement as mentioned above applies here in the
_update_episodes
method. Renaming for consistency and readability is beneficial, and it's crucial to verify that all references tosettings.show_list
are updated throughout the codebase.- 418-418: While this comment does not directly relate to the changes, it's important to note that when adding shows with defaults, error handling and logging are well implemented. However, consider abstracting the show addition logic into a separate method if it grows more complex or is reused elsewhere, to maintain modularity and readability.
sickchill/views/manage/index.py (7)
- 272-272: The change from
settings.showList
tosettings.show_list
in thedownloadSubtitleMissed
method aligns with Python's naming conventions for variable and attribute names, which recommend using lowercase with underscores for readability. This change improves consistency across the codebase.- 278-278: The usage of
settings.show_list
in thebacklogShow
method instead of the previoussettings.showList
is consistent with Python's naming conventions and enhances code readability. It's important to ensure that all references toshowList
have been updated toshow_list
throughout the entire codebase to maintain functionality.- 293-293: In the
backlogOverview
method, iterating oversettings.show_list
instead of the previoussettings.showList
is a good practice for maintaining consistency in naming conventions. This change contributes to the overall readability and maintainability of the code.- 330-330: The reference to
settings.show_list
in the sorting logic within thebacklogOverview
method is correctly updated to match the new naming convention. This ensures that the functionality related to sorting shows based on their status remains intact.- 380-380: The use of
settings.show_list
in themassEdit
method for finding shows is consistent with the updated naming convention. This change enhances code readability and maintainability by adhering to Python's recommended naming practices.- 543-543: In the
massEditSubmit
method, accessing shows usingsettings.show_list
instead of the oldsettings.showList
is in line with the updated naming convention. This change is crucial for maintaining the functionality of mass editing shows while improving code consistency.- 644-644: The reference to
settings.show_list
in themassUpdate
method for finding shows to perform various actions (update, refresh, rename, etc.) is correctly updated to match the new naming convention. This ensures that the application's manage functionality works as expected with the updated attribute names.sickchill/oldbeard/postProcessor.py (4)
- 522-522: The variable name change from
showList
toshow_list
in theShow.find
method call aligns with Python's naming conventions for variables (PEP 8), which recommend using lowercase with words separated by underscores as necessary to improve readability.- 519-525: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The file imports and dependencies are well-organized, and there's no indication of unused imports based on the provided context. It's good practice to periodically review imports to ensure they're all in use, which helps maintain code cleanliness and potentially reduce the application's memory footprint.
- 522-522: The method call
Show.find(settings.show_list, indexer_id)
suggests that theShow
class has a static or class methodfind
that operates on a list of shows. It's important to ensure that this method is robust against potential issues such as the list beingNone
, empty, or containing elements that are notShow
instances.Ensure that the
Show.find
method includes necessary error handling and validation checks to gracefully handle unexpected inputs or states.
- 522-522: Renaming
showList
toshow_list
improves readability and adheres to Python's naming conventions. However, this change should be verified across the entire codebase to ensure consistency and avoid potential reference errors.Verification successful
The search for any remaining references to the old
showList
variable name did not produce any results, suggesting that the renaming toshow_list
has been applied consistently across the Python files in the codebase. This supports the conclusion that the change adheres to Python's naming conventions and should not introduce reference errors due to inconsistent naming.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old `showList` variable name. rg --type py "showList"Length of output: 23
sickchill/views/home.py (6)
- 57-57: The method
_getEpisode
is used to retrieve an episode object based on various parameters. It's crucial to ensure that the method correctly handles all possible input scenarios and returns appropriate error messages or episode objects as expected.Ensure that the
_getEpisode
method is thoroughly tested with various input combinations to confirm its reliability and correctness.
- 91-111: The logic for constructing the
shows
andanime
lists based on theselected_root_dir
andsettings.ANIME_SPLIT_HOME
flag appears sound. However, it's essential to ensure that theshow.get_location
method is correctly implemented and that thesettings.show_list
is always up-to-date.Verify the correctness and performance of the
show.get_location
method and ensure thatsettings.show_list
is reliably maintained across the application.
- 850-850: The
displayShow
method's logic for handling various show states (e.g., being downloaded, updated, refreshed) and constructing the UI response is well-structured. However, it's important to ensure that all possible states are correctly handled and that the user interface reflects the current state accurately.Double-check the handling of all show states within the
displayShow
method to ensure comprehensive coverage and accurate UI representation.
- 1430-1430: The
updateShow
method triggers an update for a specific show. It's essential to ensure that the show update process is robust and handles failures gracefully, providing appropriate feedback to the user.Verify the error handling and user feedback mechanisms within the
updateShow
method to ensure a smooth and informative update process for users.
- 1660-1660: The
testRename
method provides a preview of the rename operation for episodes. It's important to ensure that the method accurately reflects the potential outcome of the rename operation and provides clear and useful information to the user.Verify the accuracy and usefulness of the preview generated by the
testRename
method to ensure it effectively aids users in understanding the impact of the rename operation.
- 1732-1732: The
manual_search_show_releases
method is intended to provide manual search functionality for episodes. It's important to ensure that the method correctly interacts with the cache database and accurately presents search results to the user.Verify the interaction with the cache database and the accuracy of the search results presented by the
manual_search_show_releases
method to ensure a reliable manual search experience for users.
selected_root = self.get_body_argument("root", None) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handling of selected_root
and settings.ROOT_DIRS
involves manual string manipulation and assertions. This approach might be prone to errors and could benefit from a more robust parsing and validation mechanism.
Consider using a dedicated parsing function or library to handle root directory settings and selections more safely and cleanly.
def filter(self): | ||
t = PageTemplate(rh=self, filename="home.mako") | ||
|
||
selected_root = self.get_body_argument("root", None) | ||
page = try_int(self.get_argument("p", default="0")) | ||
limit = try_int(self.get_argument("limit", default=None)) | ||
kind = self.get_argument("type", "all") | ||
genre = self.get_argument("genre", "") | ||
if kind not in ("all", "series", "anime"): | ||
kind = "all" | ||
|
||
if selected_root and settings.ROOT_DIRS: | ||
backend_pieces = settings.ROOT_DIRS.split("|") | ||
backend_dirs = backend_pieces[1:] | ||
try: | ||
assert selected_root != "-1" | ||
selected_root_dir = backend_dirs[int(selected_root)] | ||
if selected_root_dir[-1] not in ("/", "\\"): | ||
selected_root_dir += os.sep | ||
except (IndexError, ValueError, TypeError, AssertionError): | ||
selected_root_dir = "" | ||
else: | ||
shows = [] | ||
for show in settings.showList: | ||
# noinspection PyProtectedMember | ||
if selected_root_dir in show.get_location: | ||
shows.append(show) | ||
selected_root_dir = "" | ||
|
||
shows_to_show = [] | ||
skipped = 0 | ||
for show in settings.show_list: | ||
if selected_root_dir and selected_root_dir not in show.get_location: | ||
continue | ||
|
||
sorted_show_lists = [["Shows", sorted(shows, key=lambda mbr: attrgetter("sort_name")(mbr))]] | ||
if kind == "anime" and not show.is_anime: | ||
skipped += 1 | ||
continue | ||
|
||
if kind == "series" and show.is_anime: | ||
skipped += 1 | ||
continue | ||
|
||
if genre and genre.lower() not in show.genre: | ||
skipped += 1 | ||
continue | ||
|
||
shows_to_show.append(show) | ||
if limit and len(shows_to_show) == limit: | ||
break | ||
|
||
logger.debug(f"skipped {skipped} shows due to filters: genre: {genre}, limit: {limit}, kind: {kind}") | ||
if limit: | ||
upper_slice = min(page * limit + limit, len(shows_to_show)) | ||
lower_slice = min(page * limit, len(shows_to_show) - limit) | ||
|
||
number_of_pages = len(shows_to_show) // limit | ||
if len(shows_to_show) % limit: | ||
number_of_pages += 1 | ||
|
||
logger.info(f"Split home into {number_of_pages} pages") | ||
else: | ||
upper_slice = len(shows_to_show) - 1 | ||
lower_slice = 0 | ||
|
||
shows = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filter
method's implementation for filtering shows based on various criteria (e.g., root directory, page, limit, kind, genre) is comprehensive. However, the manual pagination logic (lines 160-166) could be simplified by using existing libraries or utility functions.
Consider refactoring the pagination logic to use a library or utility function that abstracts away the manual calculations for a cleaner and more maintainable approach.
@@ -1033,7 +1091,7 @@ | |||
anidb_failed = False | |||
|
|||
try: | |||
show_obj = Show.find(settings.showList, int(show)) | |||
show_obj = Show.find(settings.show_list, int(show)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The editShow
method involves complex logic for handling show editing, including updating quality settings, location, and metadata. Given the method's complexity, it's crucial to ensure that all input parameters are correctly validated and sanitized to prevent potential security issues.
Implement thorough input validation and sanitization for all parameters accepted by the editShow
method to enhance security and robustness.
@@ -1387,7 +1445,7 @@ | |||
|
|||
def subtitleShow(self): | |||
show = self.get_query_argument("show") | |||
show_obj = Show.find(settings.showList, int(show)) | |||
show_obj = Show.find(settings.show_list, int(show)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The subtitleShow
method initiates the download of subtitles for a show. Given the reliance on external services for subtitle retrieval, it's important to ensure that the method gracefully handles any errors or failures encountered during the download process.
Enhance error handling in the subtitleShow
method to provide clear and helpful feedback to users in case of download failures or issues.
@@ -1464,7 +1522,7 @@ | |||
else: | |||
return self._genericMessage(_("Error"), errMsg) | |||
|
|||
show_obj = Show.find(settings.showList, int(show)) | |||
show_obj = Show.find(settings.show_list, int(show)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setStatus
method updates the status of episodes. It's crucial to ensure that the method correctly handles the status update logic, especially when dealing with UNAIRED episodes or episodes without a valid location.
Refine the setStatus
method to include additional checks and validations to prevent inappropriate status updates for UNAIRED episodes or episodes with missing data.
@@ -1626,7 +1684,7 @@ | |||
if not (show and eps): | |||
return self._genericMessage(_("Error"), _("You must specify a show and at least one episode")) | |||
|
|||
show_obj = Show.find(settings.showList, int(show)) | |||
show_obj = Show.find(settings.show_list, int(show)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doRename
method performs the actual rename operation for episodes. Given the potential impact of this operation on user data, it's crucial to ensure that the method includes robust error handling and validation to prevent data loss or corruption.
Implement comprehensive error handling and validation within the doRename
method to safeguard against data loss or corruption during the rename operation.
@@ -1933,7 +1991,7 @@ | |||
if sceneAbsolute in ("null", ""): | |||
sceneAbsolute = None | |||
|
|||
show_obj = Show.find(settings.showList, int(show)) | |||
show_obj = Show.find(settings.show_list, int(show)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setSceneNumbering
method updates the scene numbering for episodes. Given the importance of accurate scene numbering for episode identification and sorting, it's crucial to ensure that the method correctly updates the numbering and handles any potential errors.
Enhance the setSceneNumbering
method with additional error handling and validation to ensure accurate and reliable updates to scene numbering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- sickchill/oldbeard/browser.py (1 hunks)
- sickchill/oldbeard/naming.py (4 hunks)
- sickchill/system/Restart.py (1 hunks)
- sickchill/views/api/webapi.py (27 hunks)
- tests/test_browser.py (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- sickchill/oldbeard/browser.py
- sickchill/system/Restart.py
- sickchill/views/api/webapi.py
Additional comments: 9
tests/test_browser.py (5)
- 16-16: The use of
Path(__file__)
insetUp
is a good practice for handling file paths, aligning with the PR's objective to adoptpathlib
for path handling.- 23-25: The test
test_get_win_drives
correctly checks for the presence of a common drive (C:\\
) in the list of Windows drives. This is a simple yet effective way to validate the functionality ofget_windows_drives
.- 31-32: Using
self.here / "not_a_real_path"
demonstrates the effective use ofpathlib
for constructing file paths, making the code more readable and Pythonic.- 34-40: The conditional logic for OS-specific behavior in
test_folders_at_path
is clear and well-handled. The use of"My Computer"
for Windows and"/"
for other OSes as expected paths is appropriate and demonstrates good understanding of the underlying system differences.- 44-47: The test for
include_parent=True
correctly asserts the current and parent directory paths usingpathlib
. This aligns with the PR's objective to refactor code for improved readability and consistency with modern Python practices.sickchill/oldbeard/naming.py (4)
- 55-55: Refactoring
TVEpisode
toSampleEpisode
and updating its constructor to include relevant episode information is a positive change for clarity. It's important to ensure that all references to this class throughout the codebase are updated accordingly.- 193-193: The instantiation of
SampleEpisode
withingenerate_sample_ep
is correctly updated to reflect the new class name. This change should improve clarity and maintainability of the test generation logic.- 219-219: Creating a second episode instance for multi-episode testing scenarios is a good practice. It's important to ensure that the logic for handling multiple episodes is thoroughly tested, especially with the introduction of the new
SampleEpisode
class.- 227-231: Adding a third episode instance for more complex multi-episode scenarios demonstrates a thorough approach to testing. This ensures that the application can handle various naming and episode grouping scenarios effectively.
Signed-off-by: miigotu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- sickchill/oldbeard/browser.py (1 hunks)
- sickchill/oldbeard/naming.py (4 hunks)
- sickchill/system/Restart.py (1 hunks)
- sickchill/views/api/webapi.py (27 hunks)
- tests/test_browser.py (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- sickchill/oldbeard/browser.py
- sickchill/oldbeard/naming.py
- sickchill/system/Restart.py
- sickchill/views/api/webapi.py
Additional comments: 5
tests/test_browser.py (5)
- 16-16: The initialization of
self.here
usingPath(__file__)
is a good practice, as it leveragespathlib
for path handling, aligning with the PR's objective to adoptpathlib
more consistently across the codebase.- 23-25: The test
test_get_win_drives
correctly usesbrowser.get_windows_drives()
following the method renaming for clarity. The use ofPath.cwd().drive
combined withos.sep
is a good example of integratingpathlib
with existingos
functionalities where necessary, maintaining readability and consistency.- 31-32: The assertion
assert test_list[0]["currentPath"] == str(self.here.parent)
effectively usespathlib
for path manipulation and converts it to a string for comparison. This is consistent with the move towardspathlib
and improves code readability.- 34-40: The conditional handling for different OS environments in
test_folders_at_path
is well-implemented. The use ofPath("//")
for a generic path and the subsequent assertions are clear and maintain the consistency of usingpathlib
. The loop to check if each item's path is in the drives list is straightforward and effective.- 44-47: The test
test_folders_at_path
withinclude_parent=True
parameter correctly asserts thecurrentPath
and the parent directory's name and path usingpathlib
. This aligns with the previous comment about avoiding mixingpathlib
andos.path
functionalities and demonstrates a consistent approach to path handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- sickchill/oldbeard/browser.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- sickchill/oldbeard/browser.py
… when com isn't available (ex. github actions) Signed-off-by: miigotu <[email protected]> feat: add os.listdrives from python 3.12 Signed-off-by: miigotu <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- sickchill/oldbeard/browser.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- sickchill/oldbeard/browser.py
Signed-off-by: miigotu <[email protected]>
Summary by CodeRabbit