fix: root dir index errors in mass add table and api, as well as refa… #8726
Conversation
…ctoring along the way Signed-off-by: miigotu <miigotu@gmail.com>
WalkthroughThe updates across SickChill's codebase reflect a shift towards more modern and robust path handling, using 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 22
Configuration used: CodeRabbit UI
Files selected for processing (14)
- sickchill/gui/slick/views/displayShow.mako (2 hunks)
- sickchill/oldbeard/config.py (2 hunks)
- sickchill/oldbeard/helpers.py (1 hunks)
- sickchill/oldbeard/processTV.py (5 hunks)
- sickchill/oldbeard/traktChecker.py (1 hunks)
- sickchill/show/indexers/init.py (1 hunks)
- sickchill/start.py (1 hunks)
- sickchill/tv.py (3 hunks)
- sickchill/views/api/webapi.py (5 hunks)
- sickchill/views/home.py (7 hunks)
- sickchill/views/index.py (12 hunks)
- sickchill/views/manage/add_shows.py (17 hunks)
- sickchill/views/server_settings.py (2 hunks)
- tests/test_helpers.py (2 hunks)
Files not reviewed due to errors (2)
- sickchill/start.py (Error: unable to parse review)
- sickchill/tv.py (Error: unable to parse review)
Files skipped from review due to trivial changes (2)
- sickchill/views/server_settings.py
- tests/test_helpers.py
Additional comments: 34
sickchill/show/indexers/__init__.py (1)
- 5-5: The addition of type hints improves code readability and static type checking. Good practice!
sickchill/views/index.py (4)
- 10-10: Adding
typing.Anyimport enhances type hinting throughout the file. Good addition for better type checking.- 29-32: The conditional import of
jwtandRSAAlgorithmwith a fallback toNoneis a robust way to handle optional dependencies. However, ensure that the rest of the code gracefully handles cases where these imports fail.- 169-183: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [150-180]
The
async_callmethod's refactoring to handle method arguments dynamically is a clever use of Python's introspection capabilities. However, ensure thorough testing to catch any edge cases that might arise from dynamic argument handling.
- 227-227: Improving error logging with stack information is beneficial for debugging. Ensure that sensitive information is not inadvertently logged.
sickchill/oldbeard/processTV.py (9)
- 39-39: The conversion of
folderto aPathobject is a good practice for consistent path handling across platforms. This change enhances readability and maintainability by leveraging thepathlibmodule's features.- 41-41: Correct use of
Path.is_dir()to check if the path represents a directory. This is more idiomatic and readable compared to the olderos.pathfunctions.- 50-52: Iterating over
folder.iterdir()to find files is a clean and efficient way to check for the presence of files in a directory. Logging the found files for informational purposes is a good practice for debugging and transparency.- 57-57: Using
folder.rmdir()to remove an empty directory is appropriate and aligns with the use ofpathlib. This method raises an exception if the directory is not empty, which is handled gracefully.- 63-63: The use of
shutil.rmtree()for non-empty directories is correct. However, since the rest of the function usespathlib, consider usingPath.rmdir()orPath.unlink()in a loop for consistency. Note thatshutil.rmtree()is still a valid choice for simplicity when dealing with non-empty directories.- 86-86: Converting
process_pathto aPathobject at the start of the function is consistent with the overall approach of usingpathlibfor path manipulation.- 89-90: Creating a
Pathobject for each file inunwanted_filesis a clean approach to handling file paths. This ensures compatibility with the rest of the code that has been refactored to usepathlib.- 147-147: Correctly checking if
process_pathis a directory usingPath.is_dir(). This is consistent with the rest of the refactoring to usepathlib.- 284-288: The logic to ignore hidden folders based on settings and path resolution is sound. Using
pathlibfor path resolution and comparison is consistent with the refactoring effort. However, ensure thathelpers.is_hidden_folderis compatible withPathobjects or updated accordingly.sickchill/views/manage/add_shows.py (9)
- 68-68: The condition for continuing the loop when the
indexeris specified and does not match the currentindexis clear and logical. This ensures that only the desired indexer is used for searching, improving efficiency by skipping unnecessary searches.- 92-94: The use of
sickchill.indexer[index].nameandsickchill.indexer[index].show_urlto dynamically access indexer properties is a good practice. It makes the code more flexible and easier to maintain by avoiding hard-coded references. However, ensure thatsickchill.indexeris properly initialized and contains all expected indexers to prevent runtime errors.- 111-111: The dynamic retrieval of the language ID using
sickchill.indexer[indexer].lang_dict()[lang]is a robust way to handle language-specific operations. This approach enhances maintainability by centralizing language handling within the indexer objects.- 137-137: Ignoring specific directories like
#recycleand@eadiris a practical solution to avoid processing system or service-related folders that are not relevant to the application's functionality. This helps in maintaining a clean and focused directory list for processing.- 151-151: The conversion of the query result to a boolean to determine if a directory has already been added (
cur_dir["added_already"] = bool(dir_results)) is a concise and effective way to set theadded_alreadyflag. This simplifies subsequent logic that depends on whether the directory is already part of the show list.- 230-230: The definition of
trakt_optionswith user-friendly strings for different Trakt list types is a good example of enhancing the user experience by providing meaningful descriptions for the options available to the user. This also makes the code more readable and maintainable.- 262-262: The handling of different
trakt_listvalues to set the appropriatepage_urlis logically structured and covers all expected cases. This approach ensures that the correct Trakt API endpoint is used based on the user's selection, facilitating the retrieval of relevant show data.- 396-401: The function
add_erroris a good example of encapsulating error handling logic, making the code cleaner and more maintainable. However, ensure that theadd_errorfunction is called with the correct context and that all possible error scenarios are accounted for.- 426-450: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [433-541]
The handling of show addition parameters, such as
blacklist,whitelist,anyQualities, andbestQualities, is comprehensive and well-structured. The conversion of inputs to the expected formats (e.g., convertingwhitelistandblacklistto short group names) and the handling of default values are done effectively. This ensures that the show addition process is flexible and robust, accommodating various user preferences and configurations.sickchill/oldbeard/traktChecker.py (1)
- 423-423: The change made to the indexing of
root_dirsby directly usingroot_dirs[int(root_dirs[0])]corrects the previously incorrect offset. This ensures that the correct root directory is selected based on the first element's value, which presumably holds the index of the selected root directory. This is a crucial fix for functionality but raises a question about the design choice.However, it's worth noting that this approach relies heavily on a specific format and content of the
ROOT_DIRSsetting, which might not be immediately clear to other developers or maintainers. Consider adding a comment explaining the expected format and rationale behind usingroot_dirs[0]as an index within the same array. This would improve code readability and maintainability.sickchill/oldbeard/config.py (1)
- 5-5: The addition of
pathlib.Pathis a positive change, enhancing path manipulation's robustness and readability. This aligns with modern Python practices for handling filesystem paths in a more object-oriented manner.sickchill/gui/slick/views/displayShow.mako (1)
- 4-4: The addition of
urljoinfromurllib.parseis a good practice for handling URL manipulations, ensuring compatibility and robustness in constructing URLs for downloads. This change aligns with best practices for handling file and URL paths in Python.sickchill/oldbeard/helpers.py (4)
- 950-955: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Given the removal of
real_pathandis_subdirectoryfunctions, it's crucial to verify that no other parts of the SickChill codebase rely on these functions. Removing them without ensuring they are not used elsewhere could lead to runtime errors.Verification successful
The search results indicate that the removed
real_pathandis_subdirectoryfunctions are not directly used elsewhere in the SickChill codebase. The only match found is within a test file and does not represent a direct usage of these functions. Therefore, it seems that removing these functions should not lead to runtime errors due to missing dependencies within the codebase.* 950-955: > 📝 **NOTE** > This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of `real_path` and `is_subdirectory` in the SickChill codebase. rg --type py 'real_path|is_subdirectory'Length of output: 139
The
make_contextandset_openerfunctions are related to setting up SSL context and opener for URL requests. It's important to ensure that SSL verification (SSL_VERIFY) is handled correctly to avoid security issues, especially when dealing with external requests.Ensure that the SSL verification settings (
SSL_VERIFY) are correctly applied and tested across various scenarios, including self-signed certificates and certificate chains.
- 950-955: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The
getURLfunction is a critical part of handling external HTTP requests. It's important to ensure that error handling is robust and that all potential exceptions are caught and logged appropriately to avoid unhandled exceptions that could crash the application or lead to inconsistent states.Review the error handling in the
getURLfunction to ensure it covers all potential request and response issues, including timeouts, connection errors, and unexpected response codes.
- 950-955: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The function
disk_usageuses platform-specific calls to determine disk usage, which is a good approach. However, it's important to ensure that these calls are tested on all supported platforms to avoid runtime errors or incorrect disk usage calculations.Perform thorough testing of the
disk_usagefunction on all supported operating systems to ensure accurate disk usage calculations and handle any potential exceptions or errors gracefully.sickchill/views/home.py (3)
- 75-75: The change to set
selected_rootto an empty string by default is a good practice for ensuring that the variable is always initialized, reducing the risk ofTypeErrorwhen performing operations on it.- 78-78: Splitting
settings.ROOT_DIRSand using it to setbackend_dirsis a critical operation. Ensure thatsettings.ROOT_DIRSis always in the expected format to preventIndexError. It might be beneficial to add error handling or validation forsettings.ROOT_DIRSformat before splitting.- 133-133: The filtering logic for shows based on
selected_root_dir,kind, andgenreis clear and concise. However, ensure that thegenrecomparison is always case-insensitive to avoid unexpected filtering behavior.Ensure
genrecomparison is case-insensitive across the project.sickchill/views/api/webapi.py (1)
- 608-609: The use of
Path(root_dir).resolve().is_dir()is a good practice for checking directory validity. However, ensure thatroot_diris sanitized and validated to prevent potential directory traversal vulnerabilities.Ensure
root_dirvalues are properly sanitized and validated elsewhere in the code to prevent directory traversal attacks.
| if settings.ROOT_DIRS and any( | ||
| Path(root_directory).resolve() in Path(unpack_dir).resolve().parents for root_directory in settings.ROOT_DIRS.split("|")[1:] | ||
| ): |
There was a problem hiding this comment.
The use of pathlib.Path for checking if the unpack_dir is a sub-directory of any TV root directory is a significant improvement in path handling. However, there's a potential performance consideration when settings.ROOT_DIRS contains a large number of directories, as this will perform resolve() and parents checks for each root directory. While this is typically not a concern for most use cases, it's something to be aware of in environments with extensive directory structures.
Consider caching the resolved paths of settings.ROOT_DIRS if this operation is performed frequently, to minimize the performance impact.
| for rootDir in settings.ROOT_DIRS.split('|')[1:]: | ||
| if filename.startswith(rootDir): | ||
| filename = filename.replace(rootDir, "").lstrip("/\\") | ||
| filename = urljoin(settings.DOWNLOAD_URL, quote(filename)) |
There was a problem hiding this comment.
The logic for adjusting file paths for downloads using urljoin and quote from urllib.parse is correctly implemented. However, consider adding a comment explaining the purpose of this block, especially the logic for stripping the root directory from the filename and constructing the download URL. This will enhance maintainability and readability for future contributors.
Additionally, ensure that settings.ROOT_DIRS.split('|')[1:] correctly handles all expected cases, including potential edge cases where settings.ROOT_DIRS might not be formatted as expected or could be empty.
+ # Adjust file paths for downloads by stripping the root directory and constructing the download URL
for rootDir in settings.ROOT_DIRS.split('|')[1:]:
if filename.startswith(rootDir):
filename = filename.replace(rootDir, "").lstrip("/\\")
filename = urljoin(settings.DOWNLOAD_URL, quote(filename))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.
| for rootDir in settings.ROOT_DIRS.split('|')[1:]: | |
| if filename.startswith(rootDir): | |
| filename = filename.replace(rootDir, "").lstrip("/\\") | |
| filename = urljoin(settings.DOWNLOAD_URL, quote(filename)) | |
| # Adjust file paths for downloads by stripping the root directory and constructing the download URL | |
| for rootDir in settings.ROOT_DIRS.split('|')[1:]: | |
| if filename.startswith(rootDir): | |
| filename = filename.replace(rootDir, "").lstrip("/\\") | |
| filename = urljoin(settings.DOWNLOAD_URL, quote(filename)) |
| return False | ||
|
|
||
|
|
||
| def real_path(path): | ||
| """ | ||
| Returns: | ||
| the canonicalized absolute pathname. The resulting path will have no symbolic link, '/./' or '/../' components. | ||
| """ | ||
| return os.path.normpath(os.path.normcase(os.path.realpath(path))) | ||
|
|
||
|
|
||
| def is_subdirectory(subdir_path, topdir_path): | ||
| """ | ||
| Returns true if a subdir_path is a subdirectory of topdir_path | ||
| else otherwise. | ||
|
|
||
| Parameters: | ||
| subdir_path: The full path to the subdirectory | ||
| topdir_path: The full path to the top directory to check subdir_path against | ||
| """ | ||
| topdir_path = real_path(topdir_path) | ||
| subdir_path = real_path(subdir_path) | ||
|
|
||
| # checks if the common prefix of both is equal to directory | ||
| # e.g. /a/b/c/d.rst and directory is /a/b, the common prefix is /a/b | ||
| return os.path.commonprefix([subdir_path, topdir_path]) == topdir_path | ||
|
|
||
|
|
||
| def set_up_anidb_connection(): | ||
| """Connect to anidb""" | ||
|
|
There was a problem hiding this comment.
📝 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, which is expected for a utility file like helpers.py. However, it's important to ensure that all imported modules are used within the file to avoid unnecessary imports that can lead to increased memory usage and slower startup times.
Consider reviewing the imports to ensure all are necessary and remove any that are unused.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The function make_dirs and its Windows-specific handling suggest an attempt to address platform-specific directory creation issues. However, the use of os.makedirs(path) with the exist_ok=True parameter (available in Python 3.2 and later) could simplify this logic by making it unnecessary to check if the directory exists before attempting to create it.
Consider simplifying the directory creation logic by using os.makedirs(path, exist_ok=True) to avoid platform-specific checks and manual existence verification.
- if not os.path.isdir(path):
- try:
- os.makedirs(path)
+ try:
+ os.makedirs(path, exist_ok=True)📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The use of global variables (e.g., MESSAGE_COUNTER and settings.CLIENT_WEB_URLS) for managing state within utility functions can lead to issues with thread safety and make the code harder to test and maintain.
Consider refactoring the code to reduce reliance on global state, possibly by encapsulating related functionality within classes or passing state explicitly between functions where feasible.
| # Logged into UI? | ||
| if self.get_signed_cookie("sickchill_user"): | ||
| return True | ||
|
|
||
| # Authenticate using jwt for CF Access | ||
| # NOTE: Setting a username and password is STILL required to protect poorly configured tunnels or firewalls | ||
| if settings.CF_AUTH_DOMAIN and settings.CF_POLICY_AUD and has_cryptography: | ||
| CERTS_URL = "{}/cdn-cgi/access/certs".format(settings.CF_AUTH_DOMAIN) | ||
| if "CF_Authorization" in self.request.cookies: | ||
| jwk_set = helpers.getURL(CERTS_URL, returns="json") | ||
| 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): | ||
| if settings.CF_AUTH_DOMAIN and settings.CF_POLICY_AUD and jwt: | ||
| token = self.request.cookies.get("CF_Authorization") or self.request.headers.get("Cf-Access-Jwt-Assertion") | ||
| if token: | ||
| key_set = helpers.getURL(f"{settings.CF_AUTH_DOMAIN}/cdn-cgi/access/certs", returns="json") | ||
| if key_set: | ||
| for key in key_set["keys"]: | ||
| if jwt.decode(token, key=RSAAlgorithm.from_jwk(key), audience=settings.CF_POLICY_AUD): |
There was a problem hiding this comment.
Refactoring the authentication logic to use JWT for CF Access is a significant improvement. Ensure that the RSA key is securely managed and that error handling is robust, especially in scenarios where decoding fails.
Consider adding more explicit error handling around the JWT decoding process to gracefully handle potential decoding failures or invalid tokens.
| method_argument = self.get_body_argument | ||
| method_arguments = self.get_body_arguments | ||
| elif self.request.method == "GET": | ||
| get_argument = self.get_query_argument | ||
| get_arguments = self.get_query_arguments | ||
| method_argument = self.get_query_argument | ||
| method_arguments = self.get_query_arguments | ||
| else: | ||
| get_argument = self.get_argument | ||
| get_arguments = self.get_arguments | ||
| method_argument = self.get_argument | ||
| method_arguments = self.get_arguments | ||
|
|
||
| kwargs = {} | ||
| for arg, value in self.request.arguments.items(): | ||
| if isinstance(value, str): | ||
| kwargs[arg] = get_argument(arg) | ||
| kwargs[arg] = method_argument(arg) | ||
| elif isinstance(value, list): | ||
| if len(value) == 1: | ||
| kwargs[arg] = get_argument(arg) | ||
| kwargs[arg] = method_argument(arg) | ||
| else: | ||
| kwargs[arg] = get_arguments(arg) | ||
| kwargs[arg] = method_arguments(arg) |
There was a problem hiding this comment.
The dynamic handling of method arguments based on the request method (POST/GET) is a good practice for separating concerns. However, ensure that input validation and sanitization are adequately addressed to prevent security vulnerabilities.
Consider implementing more explicit input validation and sanitization to enhance security, especially for user-supplied data.
| @@ -744,8 +738,7 @@ | |||
| root_dir = {} | |||
|
|
|||
| if settings.ROOT_DIRS: | |||
| backend_pieces = settings.ROOT_DIRS.split("|") | |||
| backend_dirs = backend_pieces[1:] | |||
| backend_dirs = settings.ROOT_DIRS.split("|")[1:] | |||
There was a problem hiding this comment.
Extracting backend_dirs from settings.ROOT_DIRS is repeated in multiple places. As mentioned earlier, consider refactoring this into a shared method to reduce code duplication and improve maintainability.
Refactor repeated backend_dirs extraction logic into a shared method.
| @@ -1496,7 +1489,7 @@ | |||
| def setStatus(self, direct=False): | |||
| if direct is True: | |||
| # noinspection PyUnresolvedReferences | |||
| show = self.to_change_showa | |||
| show = self.to_change_show | |||
There was a problem hiding this comment.
The handling of to_change_show and to_change_eps directly in the method without any prior validation or context provided in the comment raises concerns about the method's robustness and error handling. It's unclear where to_change_show and to_change_eps are coming from, which could lead to potential issues if they are not properly validated or sanitized.
Ensure to_change_show and to_change_eps are validated and sanitized before use to prevent potential issues.
| try: | ||
| os.remove(cur_file_path) | ||
| file_path.unlink(True) |
There was a problem hiding this comment.
The use of file_path.unlink(True) is incorrect. The unlink method does not take any arguments in pathlib.Path. If the intention was to force deletion, note that unlink does not have a force parameter. This line will raise a TypeError.
- file_path.unlink(True)
+ file_path.unlink()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.
| file_path.unlink(True) | |
| file_path.unlink() |
There was a problem hiding this comment.
The signature for unlink in pathlib.Path changed in python 3.8, adding a parameter miising_ok.
It is now Path.unlink(missing_ok: bool = False)
There was a problem hiding this comment.
@miigotu: Thank you for the correction. You're right, starting with Python 3.8, the Path.unlink method includes a missing_ok parameter, allowing the method to be called with an argument to avoid raising an error if the file does not exist. My previous comment was based on outdated information. I appreciate your clarification.
There was a problem hiding this comment.
In python 3.8+ pathlib.Path.unlink takes a missong_ok:bool parameter
There was a problem hiding this comment.
@miigotu: Absolutely, you're correct. The missing_ok parameter in Path.unlink for Python 3.8+ allows for more flexible file handling, especially in scenarios where the absence of a file should not raise an error. Thank you for your patience and for clarifying this important detail. Your expertise is greatly appreciated!
| [ | ||
| settings.TV_DOWNLOAD_DIR, | ||
| Path(settings.TV_DOWNLOAD_DIR).is_dir(), | ||
| str(Path(process_path).resolve()) == str(Path(settings.TV_DOWNLOAD_DIR).resolve()), | ||
| ] |
There was a problem hiding this comment.
The logic to check if the client and SickChill are on the same machine and to translate the directory into a network directory is thoughtful. However, the conversion of process_path to a Path object and then back to a string using os.path.join is inconsistent. Since the codebase is being refactored to use pathlib, consider using pathlib operations throughout for consistency.
- process_path = os.path.join(settings.TV_DOWNLOAD_DIR, os.path.abspath(process_path).split(os.path.sep)[-1])
+ process_path = Path(settings.TV_DOWNLOAD_DIR) / Path(process_path).nameCommittable 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.
| [ | |
| settings.TV_DOWNLOAD_DIR, | |
| Path(settings.TV_DOWNLOAD_DIR).is_dir(), | |
| str(Path(process_path).resolve()) == str(Path(settings.TV_DOWNLOAD_DIR).resolve()), | |
| ] | |
| [ | |
| settings.TV_DOWNLOAD_DIR, | |
| Path(settings.TV_DOWNLOAD_DIR).is_dir(), | |
| str(Path(process_path).resolve()) == str(Path(settings.TV_DOWNLOAD_DIR).resolve()), | |
| ] |
| indexer_id = self.get_body_argument("indexerId") | ||
| image_url = sickchill.indexer.series_poster_url_by_id(indexer_id) | ||
| if image_url: | ||
| image_path = trakt_trending.get_image_path(trakt_trending.get_image_name(indexerId)) | ||
| image_path = trakt_trending.get_image_path(trakt_trending.get_image_name(indexer_id)) | ||
| trakt_trending.cache_image(image_url, image_path) | ||
| return indexerId | ||
| return indexer_id |
There was a problem hiding this comment.
Tip
Codebase Verification
The review comment is valid. The provided code snippet in add_shows.py and the related cache_image method implementation do not include explicit error handling for the image download and caching process. This could potentially break the user flow if exceptions occur during these operations, aligning with the concerns raised in the review comment. Adding error handling around these operations would be beneficial to ensure a smoother user experience.
Analysis chain
Fetching the series poster URL using sickchill.indexer.series_poster_url_by_id(indexer_id) and caching the image if available is a neat feature. However, ensure that error handling is in place for cases where image_url might not be retrievable or if caching fails, to prevent breaking the user flow.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify error handling for image retrieval and caching
grep -A 5 -B 5 "cache_image" .
Length of output: 54
Script:
#!/bin/bash
# Use ripgrep to search for cache_image usage and its error handling
rg "cache_image" -C 5
Length of output: 8809
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- sickchill/show_updater.py (1 hunks)
Additional comments: 2
sickchill/show_updater.py (2)
- 9-9: The import statement for the
Showclass has been updated. Ensure that this new import path is correct and that there are no issues with circular dependencies or broken functionalities as a result of this change. It's also important to verify that all instances where theShowclass is used within this file (and potentially others) have been updated to reflect this change.- 6-12: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [10-89]
The
ShowUpdaterclass appears to be well-structured and follows good practices in terms of error handling and logic separation. However, given the change in the import statement for theShowclass, it's crucial to ensure that all interactions with theShowclass within this module are tested thoroughly to prevent any runtime errors or unexpected behavior. This includes verifying the methodsupdateandrefreshcalled onShowinstances to ensure they are compatible with the updated import path.
| if not settings.ROOT_DIRS: | ||
| return {} | ||
|
|
||
| root_dir = {} | ||
| root_dirs = settings.ROOT_DIRS.split("|") | ||
| default_index = int(settings.ROOT_DIRS.split("|")[0]) | ||
|
|
||
| root_dir["default_index"] = int(settings.ROOT_DIRS.split("|")[0]) | ||
| # remove default_index value from list (this fixes the offset) | ||
| root_dirs.pop(0) | ||
|
|
||
| if len(root_dirs) < default_index: | ||
| try: | ||
| default_index = int(root_dirs[0]) | ||
| except ValueError: | ||
| return {} | ||
|
|
||
| # clean up the list - replace %xx escapes by their single-character equivalent | ||
| root_dirs = [urllib.parse.unquote_plus(x) for x in root_dirs] | ||
| if default_index > len(root_dirs) - 1: | ||
| return {} |
There was a problem hiding this comment.
The error handling for the _get_root_dirs function could be improved by providing more specific error messages or handling scenarios where the default_index is not a valid integer more gracefully.
Consider adding more descriptive error handling or a fallback mechanism for invalid default_index values.
|
|
||
| # disallow adding/setting an invalid dir | ||
| if not os.path.isdir(self.location): | ||
| if not Path(self.location).is_dir(): |
There was a problem hiding this comment.
The validation check for the directory's existence is crucial for security and correctness. However, consider adding more specific error handling or logging to provide feedback on why the operation failed if the directory does not exist.
Enhance error handling or logging to provide more specific feedback when the directory validation fails.
| if settings.ROOT_DIRS: | ||
| root_dirs = settings.ROOT_DIRS.split("|") | ||
| root_dirs.pop(0) | ||
| default_index = int(settings.ROOT_DIRS.split("|")[0]) | ||
| default_index = int(root_dirs[0]) | ||
| if default_index > len(root_dirs) - 1: | ||
| return _responds(RESULT_FAILURE, msg="Default root directory is not set, please provide a location") | ||
|
|
||
| self.location = root_dirs[default_index] | ||
| else: | ||
| return _responds(RESULT_FAILURE, msg="Root directory is not set, please provide a location") | ||
|
|
||
| if not os.path.isdir(self.location): | ||
| if not Path(self.location).is_dir(): |
There was a problem hiding this comment.
Similar to previous comments, the logic for setting the location for a new show involves checking and manipulating global settings and paths. This could be simplified and made more robust.
Consider encapsulating the logic related to determining the show's location into a separate function or method to improve readability and maintainability.
Signed-off-by: miigotu <miigotu@gmail.com>
| class RootDirectories: | ||
| def __init__(self, root_directories_string: str = "", root_directories_list: Union[list, None] = None) -> None: | ||
| self.default_root_index: Union[int, None] = None | ||
| self.root_directories: list = [] | ||
|
|
||
| self.parse(root_directories_string or settings.ROOT_DIRS, root_directories_list) | ||
| logger.debug(f"parsed roots: {[str(directory) for directory in self]} from {settings.ROOT_DIRS}") | ||
| if self.default: | ||
| logger.debug(f"default root directory: {self[self.default]}") | ||
| else: | ||
| logger.debug(f"no default root directory set") | ||
|
|
||
| def parse(self, root_directories_string: str = "", root_directories_list: Union[list, None] = None) -> None: | ||
| split_setting = root_directories_list or root_directories_string.split("|") | ||
|
|
||
| if len(split_setting) < 2: | ||
| return | ||
|
|
||
| self.default_root_index = int(split_setting[0]) | ||
| for root_directory in split_setting[1:]: | ||
| self.add(root_directory) | ||
|
|
||
| def add(self, root_directory: Union[str, Path]) -> bool: | ||
| root_directory = Path(root_directory).resolve() | ||
| if not root_directory.is_dir(): | ||
| logger.debug(f"tried to add a non-existent root directory: {root_directory}") | ||
| return False | ||
|
|
||
| if root_directory in self.root_directories: | ||
| logger.debug(f"tried to add a root directory that is already added: {root_directory}") | ||
| return False | ||
|
|
||
| logger.debug(f"adding root directory {root_directory}") | ||
| self.root_directories.append(root_directory) | ||
|
|
||
| self.__check_default_index() | ||
|
|
||
| return True | ||
|
|
||
| def delete(self, root_directory: Union[str, Path]) -> bool: | ||
| root_directory = Path(root_directory) | ||
| if root_directory in self.root_directories: | ||
| self.root_directories.remove(root_directory) | ||
| self.__check_default_index() | ||
| return True | ||
| return False | ||
|
|
||
| @property | ||
| def default(self) -> int: | ||
| return self.default_root_index | ||
|
|
||
| @default.setter | ||
| def default(self, value: Union[int, str]) -> None: | ||
| value = int(value) | ||
| self.default_root_index = value | ||
| self.__check_default_index() | ||
|
|
||
| def info(self) -> list: | ||
| output = [] | ||
| for root_dir in self: | ||
| output.append({"valid": root_dir.is_dir(), "location": str(root_dir), "default": int(root_dir is self[self.default])}) | ||
|
|
||
| return output or {} | ||
|
|
||
| def __check_default_index(self): | ||
| if not self.root_directories: | ||
| self.default_root_index = None | ||
| return | ||
|
|
||
| if not self.default_root_index or self.default_root_index > len(self.root_directories): | ||
| self.default_root_index = 1 | ||
|
|
||
| def __str__(self) -> str: | ||
| return "|".join(f"{item}" for item in [self.default_root_index] + self.root_directories) | ||
|
|
||
| def __getitem__(self, item: int) -> Path: | ||
| return self.root_directories[item - 1] | ||
|
|
||
| def __setitem__(self, index: int, value: Path): | ||
| self.root_directories[index - 1] = value | ||
| self.__check_default_index() | ||
|
|
||
| def __delitem__(self, key): | ||
| self.root_directories.pop(key - 1) | ||
| self.__check_default_index() | ||
|
|
||
| def __contains__(self, item): | ||
| return item in self.root_directories | ||
|
|
||
| def __iter__(self): | ||
| for item in self.root_directories: | ||
| yield item | ||
|
|
||
| def __len__(self): | ||
| return len(self.root_directories) |
There was a problem hiding this comment.
The RootDirectories class implementation is generally well-structured and makes good use of pathlib.Path for path manipulation. However, there are several areas where improvements can be made for clarity, efficiency, and robustness:
- Error Handling in
addMethod: The method logs a debug message when trying to add a non-existent directory but does not raise an exception. Consider raising an exception or returning a more explicit error to the caller to handle this case more robustly. - Default Root Directory Index Handling: The logic for setting and checking the default root directory index (
default_root_index) could be simplified or documented more clearly. The use of1as a default index in__check_default_index(line 77) is not immediately intuitive since Python lists are zero-indexed. - Return Value of
infoMethod: Theinfomethod returns an empty dictionary ({}) if there are no root directories (line 69). This is inconsistent with the expected return type of a list. Consider returning an empty list ([]) for consistency. - Magic Numbers: The use of magic numbers, such as the default root index
1in__check_default_index, could be replaced with named constants for clarity. - Efficiency in
__str__Method: The__str__method constructs the string representation by iterating overself.root_directoriesand converting eachPathobject to a string. This is efficient, but ensure that this method's performance is acceptable for the expected size ofself.root_directories.
Overall, the class provides a solid foundation for managing root directories, but addressing these points could enhance its clarity, robustness, and maintainability.
| self.assertEqual(self.object.default_root_index, self.initial_index) | ||
|
|
||
| self.object.add(tempfile.tempdir) | ||
|
|
||
| self.assertNotEqual(len(self.object.root_directories), self.initial_length) | ||
| self.assertNotEqual(self.object.root_directories, self.initial_list) | ||
| self.assertEqual(self.object.default_root_index, self.initial_index) | ||
|
|
||
| def test_delete(self): | ||
| self.object.delete(self.path_one) | ||
| self.assertLess(len(self.object), self.initial_length) | ||
|
|
||
| length = len(self.object) | ||
| self.object.delete(self.path_one) | ||
| self.assertEqual(len(self.object), length) | ||
| self.assertIsNotNone(self.object.default_root_index) | ||
| self.assertEqual(self.object.default_root_index, self.initial_index) | ||
|
|
||
| self.object.delete(self.path_two) | ||
| self.assertFalse(len(self.object)) | ||
|
|
||
| self.assertIsNone(self.object.default_root_index) | ||
|
|
||
| def test_default(self): | ||
| self.object.add(tempfile.tempdir) | ||
| self.assertEqual(self.object.default, self.initial_index) | ||
|
|
||
| self.object.default = 10 | ||
| self.assertEqual(self.object.default, self.initial_index) | ||
| self.assertEqual(self.object.default, self.initial_index) | ||
|
|
||
| self.object.delete(self.path_one) | ||
| self.object.delete(self.path_two) | ||
| self.object.delete(tempfile.tempdir) | ||
| self.assertIsNone(self.object.default, self.object.root_directories) | ||
|
|
||
| self.object.add(tempfile.tempdir) | ||
| self.assertEqual(self.object.default, self.initial_index) | ||
|
|
||
| def test_info(self): | ||
| info = self.object.info() | ||
| self.assertEqual(len(info), len(self.object)) | ||
|
|
||
| self.assertIn("valid", info[0]) | ||
| self.assertEqual(info[0]["valid"], True) | ||
|
|
||
| self.assertIn("default", info[0]) | ||
| self.assertEqual(info[0]["default"], True) | ||
|
|
||
| self.assertEqual(info[1]["default"], False) | ||
| self.assertEqual(info[0]["location"], str(self.path_one)) | ||
|
|
||
| self.object.delete(self.path_one) | ||
| self.object.delete(self.path_two) | ||
|
|
||
| info = self.object.info() | ||
| self.assertEqual(len(info), len(self.object)) | ||
| self.assertEqual(info, {}) | ||
|
|
||
| def test_str(self): | ||
| self.assertEqual(self.object.__str__(), self.settings_root_dirs) | ||
|
|
||
| self.object.add(tempfile.tempdir) | ||
| self.assertNotEqual(self.object.__str__(), self.settings_root_dirs) | ||
|
|
||
| self.object.delete(tempfile.tempdir) | ||
| self.assertEqual(self.object.__str__(), self.settings_root_dirs) | ||
|
|
||
| def test_getitem(self): | ||
| self.assertEqual(self.object[1], self.path_one) | ||
| self.assertEqual(self.object[1], self.object.root_directories[0]) | ||
|
|
||
| def test_setitem(self): | ||
| self.assertEqual(self.object[1], self.path_one) | ||
| self.object[1] = self.path_two | ||
| self.assertEqual(self.object[1], self.path_two) | ||
|
|
||
| def test_del(self): | ||
| del self.object[1] | ||
| self.assertEqual(self.object[1], self.path_two) | ||
| self.assertLess(len(self.object), self.initial_length) | ||
|
|
||
| def test_contains(self): | ||
| self.assertFalse(tempfile.tempdir in self.object) | ||
| self.assertTrue(self.path_one in self.object) | ||
| self.assertTrue(self.path_two in self.object) | ||
|
|
||
| self.object.delete(self.path_one) | ||
| self.assertFalse(self.path_one in self.object) | ||
| self.assertTrue(self.path_two in self.object) | ||
|
|
||
| def test_iter(self): | ||
| object_list = [] | ||
| for item in self.object: | ||
| object_list.append(item) | ||
|
|
||
| self.assertEqual(object_list, self.object.root_directories) | ||
|
|
||
| def test_len(self): | ||
| self.assertEqual(len(self.object), len(self.object.root_directories)) |
There was a problem hiding this comment.
The test suite for the RootDirectories class is comprehensive and covers a wide range of scenarios, including adding, deleting, and retrieving root directories. However, there are a few areas where improvements can be made:
- Use of
tempfile.tempdir: While usingtempfile.tempdiris convenient, it's a shared resource that could potentially lead to conflicts when running tests in parallel or with other tests that modify this directory. Consider usingtempfile.TemporaryDirectoryas a context manager to create and clean up temporary directories explicitly for each test where it's needed. - Duplication in
test_defaultMethod: Thetest_defaultmethod (lines 88-103) contains duplicated assertions (lines 92-94). Remove the redundant assertion to clean up the test. - Asserting Empty Collections: The test
test_infochecks ifinforeturns{}when there are no root directories (line 122). As mentioned in the review ofsickchill/helper/rootdirs.py,infoshould return an empty list instead of an empty dictionary for consistency. This test should be updated accordingly once the method's return value is corrected. - Test Coverage for Edge Cases: Consider adding tests for edge cases, such as adding a root directory that is not a directory (e.g., a file) or setting a negative index as the default root directory. These cases can help ensure the robustness of the
RootDirectoriesclass.
Overall, the test suite is well-structured and provides good coverage of the RootDirectories class's functionality. Addressing these points could further improve its comprehensiveness and reliability.
…ctoring along the way
Fixes #
Proposed changes in this pull request:
PR is based on the DEVELOP branch
Don't send big changes all at once. Split up big PRs into multiple smaller PRs that are easier to manage and review
Read contribution guide
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests