mass refactor tv.py #8706
mass refactor tv.py #8706
Conversation
…in tv.py Signed-off-by: miigotu <[email protected]>
…ing errors Signed-off-by: miigotu <[email protected]>
WalkthroughThe recent updates across various files in the project focus on modernizing the codebase through the adoption of f-strings for improved string formatting, refining method naming conventions for consistency, enhancing error handling and logging for better debuggability, and updating method calls to adhere to new naming standards. Additionally, there's an emphasis on code readability and maintainability, alongside specific functional adjustments like improved URL generation logic in JavaScript and the introduction of new parameters in file processing methods. 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 (
|
Signed-off-by: miigotu <[email protected]>
![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: 74
Configuration used: CodeRabbit UI
Files selected for processing (37)
- SickChill.py (5 hunks)
- sickchill/gui/slick/views/config_providers.mako (34 hunks)
- sickchill/oldbeard/classes.py (1 hunks)
- sickchill/oldbeard/dailysearcher.py (1 hunks)
- sickchill/oldbeard/failedProcessor.py (1 hunks)
- sickchill/oldbeard/helpers.py (1 hunks)
- sickchill/oldbeard/nzbSplitter.py (1 hunks)
- sickchill/oldbeard/postProcessor.py (2 hunks)
- sickchill/oldbeard/properFinder.py (1 hunks)
- sickchill/oldbeard/providers/bjshare.py (1 hunks)
- sickchill/oldbeard/scene_numbering.py (1 hunks)
- sickchill/oldbeard/search.py (4 hunks)
- sickchill/oldbeard/searchBacklog.py (1 hunks)
- sickchill/oldbeard/show_queue.py (9 hunks)
- sickchill/oldbeard/subtitles.py (1 hunks)
- sickchill/oldbeard/traktChecker.py (2 hunks)
- sickchill/oldbeard/tvcache.py (2 hunks)
- sickchill/providers/GenericProvider.py (4 hunks)
- sickchill/providers/metadata/generic.py (5 hunks)
- sickchill/providers/torrent/FrenchProvider.py (2 hunks)
- sickchill/providers/torrent/TorrentProvider.py (1 hunks)
- sickchill/show/ComingEpisodes.py (1 hunks)
- sickchill/show/History.py (2 hunks)
- sickchill/show/Show.py (1 hunks)
- sickchill/show_updater.py (1 hunks)
- sickchill/start.py (5 hunks)
- sickchill/tv.py (72 hunks)
- sickchill/views/api/webapi.py (6 hunks)
- sickchill/views/home.py (9 hunks)
- sickchill/views/manage/index.py (2 hunks)
- tests/conftest.py (2 hunks)
- tests/sickchill_tests/show/test_show.py (1 hunks)
- tests/test_notifier.py (2 hunks)
- tests/test_pp.py (1 hunks)
- tests/test_search.py (2 hunks)
- tests/test_snatch.py (1 hunks)
- tests/test_tv.py (5 hunks)
Files not summarized due to errors (1)
- sickchill/tv.py: Error: Message exceeds token limit
Files not reviewed due to errors (1)
- sickchill/views/api/webapi.py (Error: unable to parse review)
Files skipped from review due to trivial changes (3)
- sickchill/oldbeard/providers/bjshare.py
- sickchill/oldbeard/scene_numbering.py
- sickchill/oldbeard/search.py
Additional comments: 65
sickchill/oldbeard/failedProcessor.py (1)
- 59-59: Method call
getEpisodehas been correctly updated toget_episodeto adhere to Python's naming conventions.sickchill/providers/torrent/FrenchProvider.py (4)
- 65-66: The conversion of
_get_custom_urland_set_custom_urlinto acustom_urlproperty is correctly implemented.- 69-70: The setter for
custom_urlis correctly implemented, ensuring_recheck_urlis set toTruewhen a new URL is assigned.- 75-76: The
urlproperty correctly checks for a valid custom URL before returning it, enhancing error handling.- 88-89: The setter for
urlis correctly implemented, directly setting the_used_url.tests/test_tv.py (8)
- 27-27: Method call
loadFromDBhas been correctly updated toload_from_dbto adhere to Python's naming conventions.- 34-34: Method call
loadFromDBhas been correctly updated toload_from_dbto adhere to Python's naming conventions.- 38-38: Method call
saveToDBhas been correctly updated tosave_to_dbto adhere to Python's naming conventions.- 49-49: Method call
saveToDBhas been correctly updated tosave_to_dbto adhere to Python's naming conventions.- 56-56: Method call
saveToDBhas been correctly updated tosave_to_dbto adhere to Python's naming conventions.- 67-67: Method call
saveToDBhas been correctly updated tosave_to_dbto adhere to Python's naming conventions.- 95-95: Method call
saveToDBhas been correctly updated tosave_to_dbto adhere to Python's naming conventions.- 126-126: Method call
saveToDBhas been correctly updated tosave_to_dbto adhere to Python's naming conventions.tests/sickchill_tests/show/test_show.py (1)
- 121-121: Method call
loadFromDBhas been correctly updated toload_from_dbto adhere to Python's naming conventions.sickchill/providers/torrent/TorrentProvider.py (1)
- 34-34: Method call
getEpisodehas been correctly updated toget_episodeto adhere to Python's naming conventions.sickchill/oldbeard/dailysearcher.py (1)
- 69-69: Method call
getEpisodehas been correctly updated toget_episodeto adhere to Python's naming conventions.tests/test_snatch.py (2)
- 118-118: Method call
saveToDBhas been correctly updated tosave_to_dbto adhere to Python's naming conventions.- 125-125: Method call
saveToDBhas been correctly updated tosave_to_dbto adhere to Python's naming conventions.sickchill/show_updater.py (1)
- 53-53: Method call
nextEpisodehas been correctly updated tonext_episodeto adhere to Python's naming conventions.tests/test_search.py (2)
- 47-47: Method call
saveToDBhas been correctly updated tosave_to_dbto adhere to Python's naming conventions.- 58-58: Method call
saveToDBhas been correctly updated tosave_to_dbto adhere to Python's naming conventions.tests/test_pp.py (2)
- 53-53: Method call
saveToDBhas been correctly updated tosave_to_dbto adhere to Python's naming conventions.- 58-58: Method call
saveToDBhas been correctly updated tosave_to_dbto adhere to Python's naming conventions.sickchill/oldbeard/searchBacklog.py (1)
- 142-142: Method call
getEpisodehas been correctly updated toget_episodeto adhere to Python's naming conventions.sickchill/show/ComingEpisodes.py (1)
- 79-79: Method call
nextEpisodehas been correctly updated tonext_episodeto adhere to Python's naming conventions.SickChill.py (4)
- 4-4:
import mimetypesis added without removing any existing imports, ensuring no conflicts or redundancies.- 69-69:
self.flask_server = Noneis correctly initialized in the__init__method of theSickChillclass, aligning with the summary.- 257-257: Renaming
cur_show.nextEpisode()tocur_show.next_episode()inload_shows_from_dbmethod correctly follows Python's naming conventions for methods.- 305-305: Renaming
sickchill.start.saveAll()tosickchill.start.save_all()in theshutdownmethod correctly follows Python's naming conventions for methods.sickchill/show/History.py (2)
- 315-315: The change from
saveToDB()tosave_to_db()in therevert_episodemethod correctly follows Python's naming conventions for methods.- 335-335: The change from
saveToDB()tosave_to_db()in themark_failedmethod correctly follows Python's naming conventions for methods.sickchill/oldbeard/tvcache.py (1)
- 422-422: The parameter name changes from
manualSearchtomanual_searchanddownCurQualitytodown_cur_qualityin thefind_needed_episodesmethod correctly follow Python's naming conventions for function arguments.sickchill/oldbeard/subtitles.py (1)
- 449-449: The method call
show_object.get_episode(ep_season, ep_episode)has been correctly updated fromgetEpisodetoget_episodeto adhere to the new naming convention.sickchill/providers/GenericProvider.py (3)
- 284-284: The method call
show_object.want_episodehas been correctly updated fromwantEpisodetowant_episodeto adhere to the new naming convention.- 296-296: The method call
show_object.get_episodehas been correctly updated fromgetEpisodetoget_episodeto adhere to the new naming convention.- 636-636: The method
has_optionwas removed as part of the refactor. Ensure that its removal does not affect any functionality that depended on it. This change might require a broader codebase verification to confirm there are no calls tohas_optionleft.Verification successful
The executed shell script aimed to find any occurrences of
has_optionwithin Python files in the codebase. The script did not produce any output, indicating that there are no calls tohas_optionpresent. This suggests that the removal of thehas_optionmethod, as part of the refactor, does not leave behind any unresolved references to it in the codebase.Based on this information, it can be concluded that the removal of the
has_optionmethod does not affect any functionality that depended on it, as there are no remaining calls to this method.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any calls to `has_option` in the codebase. rg --type py 'has_option'Length of output: 25
sickchill/oldbeard/show_queue.py (14)
- 483-483: Method
save_to_dbis correctly renamed fromsaveToDBto adhere to Python naming conventions.- 495-495: Method
load_episodes_from_indexeris correctly renamed fromloadEpisodesFromIndexerto adhere to Python naming conventions.- 504-504: Method
load_episodes_from_diris correctly renamed fromloadEpisodesFromDirto adhere to Python naming conventions.- 515-517: Methods
write_metadata,update_metadata, andpopulate_cacheare correctly renamed fromwriteMetadata,updateMetadata, andpopulateCacherespectively to adhere to Python naming conventions.- 519-519: Method
flush_episodesis correctly renamed fromflushEpisodesto adhere to Python naming conventions.- 568-572: Methods
refresh_dir,write_metadata,update_metadata, andpopulate_cacheare correctly renamed from their camelCase counterparts to adhere to Python naming conventions.- 600-600: Method
get_all_episodesis correctly renamed fromgetAllEpisodesto adhere to Python naming conventions.- 658-658: Method
save_to_dbis correctly renamed fromsaveToDBto adhere to Python naming conventions.- 664-664: Method
load_episodes_from_dbis correctly renamed fromloadEpisodesFromDBto adhere to Python naming conventions.- 669-669: Method
load_episodes_from_indexeris correctly renamed fromloadEpisodesFromIndexerto adhere to Python naming conventions.- 677-678: Method
get_episodeis correctly renamed fromgetEpisodeto adhere to Python naming conventions.- 687-689: Method
delete_episodeis correctly renamed fromdeleteEpisodeto adhere to Python naming conventions.- 695-695: Method
save_to_dbis correctly renamed fromsaveToDBto adhere to Python naming conventions.- 719-719: Method
delete_showis correctly renamed fromdeleteShowto adhere to Python naming conventions.sickchill/oldbeard/traktChecker.py (2)
- 597-597: Method
get_episodeis correctly renamed fromgetEpisodeto adhere to Python naming conventions.- 607-607: Method
save_to_dbis correctly renamed fromsaveToDBto adhere to Python naming conventions.sickchill/views/manage/index.py (2)
- 267-267: Method call
show.get_episodeshould be reviewed for correctness in the context of the refactor.- 308-308: Method call
current_show.get_overviewshould be reviewed for correctness in the context of the refactor.sickchill/providers/metadata/generic.py (5)
- 118-118: Method
has_episode_metadatashould be reviewed for access level consistency.- 130-130: Method
has_episode_thumbshould be reviewed for access level consistency.- 266-266: Method call
self.write_ep_fileshould be reviewed for correctness in the context of the refactor.- 272-272: Method call
self.has_episode_metadatashould be reviewed for correctness in the context of the refactor.- 345-345: Method call
self.has_episode_thumbshould be reviewed for correctness in the context of the refactor.sickchill/oldbeard/helpers.py (1)
- 662-662: The method call has been correctly updated from
show.getEpisode()toshow.get_episode()to align with the new naming convention. Ensure that theShowclass or any of its subclasses whereget_episodeis defined have been updated accordingly to reflect this new method name.sickchill/start.py (4)
- 127-137: Parameter names in
restore_cachefunction have been updated for clarity. Ensure all calls to this function reflect the new parameter names.Verification successful
Based on the provided context and the output from the executed script, it's clear that the
restore_cachefunction is indeed called within thesickchill/start.pyfile, using the updated parameter names as indicated in the snippet provided in the review comment. The script specifically searched for calls torestore_cachewith any two parameters, and it found a match, indicating that the function is being used with the new parameter names. This confirms that the changes mentioned in the review comment have been correctly applied in the codebase.Therefore, the review comment regarding the need to ensure all calls to the
restore_cachefunction reflect the new parameter names has been verified as correct based on the script output showing the function call with the updated parameter names.* 786-838: The handling of provider attributes in the initialization block uses a consistent pattern for checking attributes and setting values. Ensure that all provider attributes are correctly handled according to their data types and that any new provider attributes introduced are also initialized here. * 1097-1150: When saving provider settings to the configuration, passwords and sensitive information are encrypted, which is good practice. However, ensure that all sensitive fields are indeed encrypted and that any new sensitive fields introduced are also encrypted. * 1703-1706: The `launchBrowser` function attempts to open a web browser to the SickChill home page. The use of `webbrowser.open` with different flags is a good fallback mechanism. Ensure that this function is called appropriately and that the `web_root` and `startPort` parameters are correctly passed throughout the application.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for `restore_cache` function calls with old parameter names. ast-grep --lang python --pattern $'restore_cache($_, $_)'Length of output: 169
sickchill/tv.py (1)
- 136-136: Use of
Show.findmethod without context. Verify existence and correct usage.Verification successful
The script output provides extensive references to both
settings.showListandself.indexeridacross various files in the SickChill project. This indicates thatsettings.showListis widely used throughout the project, likely as a collection of show objects managed by the application. The references span across different modules, including views, helpers, and the main SickChill functionality, suggesting thatsettings.showListis a central part of the application's state, holding the list of shows SickChill is managing or tracking.Similarly,
self.indexeridis referenced across a broad range of contexts, primarily within thesickchill/tv.pyfile but also in other parts of the application. This suggests thatself.indexeridis a fundamental attribute of the class it's used in, likely representing a unique identifier for a show within the SickChill application or an external indexing service.Given the widespread use of both
settings.showListandself.indexeridin contexts that align with the expected usage of theShow.findmethod (i.e., searching for a show within a list of shows by its unique identifier), it's reasonable to conclude that the usage ofShow.find(settings.showList, self.indexerid)in the provided code snippet is correct and appropriate for the intended functionality.The method
Show.findis designed to search through a list of shows (settings.showList) for a show with a specific indexer ID (self.indexerid). The references found in the script output confirm thatsettings.showListis indeed a list of shows and thatself.indexeridis used as a unique identifier for shows, which matches the method's parameters and intended use.Based on this analysis, the use of
Show.findin the provided code snippet is verified to be correct and consistent with the application's design and usage patterns.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence and usage of Show.find method grep -r "def find(" .Length of output: 106
Script:
#!/bin/bash # Verify the type and initialization of settings.showList and self.indexerid grep -r "settings.showList" . grep -r "self.indexerid" .Length of output: 28882
| @@ -164,15 +164,15 @@ def split_result(obj): | |||
|
|
|||
| want_ep = True | |||
| for ep_num in parsed_obj.episode_numbers: | |||
| if not obj.show.wantEpisode(season, ep_num, obj.quality): | |||
| if not obj.show.want_episode(season, ep_num, obj.quality): | |||
There was a problem hiding this comment.
Method call want_episode should be wantEpisode to match the updated naming convention.
- if not obj.show.want_episode(season, ep_num, obj.quality):
+ if not obj.show.wantEpisode(season, ep_num, obj.quality):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.
| if not obj.show.want_episode(season, ep_num, obj.quality): | |
| if not obj.show.wantEpisode(season, ep_num, obj.quality): |
| logger.debug("Ignoring result: " + new_nzb) | ||
| want_ep = False | ||
| break | ||
| if not want_ep: | ||
| continue | ||
|
|
||
| # get all the associated episode objects | ||
| ep_obj_list = [obj.show.getEpisode(season, ep) for ep in parsed_obj.episode_numbers] | ||
| ep_obj_list = [obj.show.get_episode(season, ep) for ep in parsed_obj.episode_numbers] |
There was a problem hiding this comment.
Method call get_episode should be getEpisode to match the updated naming convention.
- ep_obj_list = [obj.show.get_episode(season, ep) for ep in parsed_obj.episode_numbers]
+ ep_obj_list = [obj.show.getEpisode(season, ep) for ep in parsed_obj.episode_numbers]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.
| ep_obj_list = [obj.show.get_episode(season, ep) for ep in parsed_obj.episode_numbers] | |
| ep_obj_list = [obj.show.getEpisode(season, ep) for ep in parsed_obj.episode_numbers] |
| @@ -76,7 +76,7 @@ def make_result(cls, result_dict): | |||
| return show[0] | |||
|
|
|||
| show = show[1] | |||
| episode_objects = [show.getEpisode(result_dict.get("season"), ep) for ep in result_dict.get("episodes").split("|") if ep] | |||
| episode_objects = [show.get_episode(result_dict.get("season"), ep) for ep in result_dict.get("episodes").split("|") if ep] | |||
There was a problem hiding this comment.
Method call get_episode should be getEpisode to match the updated naming convention.
- episode_objects = [show.get_episode(result_dict.get("season"), ep) for ep in result_dict.get("episodes").split("|") if ep]
+ episode_objects = [show.getEpisode(result_dict.get("season"), ep) for ep in result_dict.get("episodes").split("|") if ep]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.
| episode_objects = [show.get_episode(result_dict.get("season"), ep) for ep in result_dict.get("episodes").split("|") if ep] | |
| episode_objects = [show.getEpisode(result_dict.get("season"), ep) for ep in result_dict.get("episodes").split("|") if ep] |
| @@ -161,7 +161,7 @@ def pause(indexer_id, pause=None): | |||
| else: | |||
| show.paused = pause | |||
|
|
|||
| show.saveToDB() | |||
| show.save_to_db() | |||
There was a problem hiding this comment.
Method call save_to_db should be saveToDB to match the updated naming convention.
- show.save_to_db()
+ show.saveToDB()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.
| show.save_to_db() | |
| show.saveToDB() |
| @@ -44,7 +44,7 @@ def setUpClass(cls): | |||
| episode.name = f"Episode {episode_counter + 1}" | |||
| episode.quality = "SDTV" | |||
| show.episodes.append(episode) | |||
| show.saveToDB() | |||
| show.save_to_db() | |||
There was a problem hiding this comment.
Method call save_to_db should be saveToDB to match the updated naming convention.
- show.save_to_db()
+ show.saveToDB()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.
| show.save_to_db() | |
| show.saveToDB() |
| settings.showList = [self.show] | ||
|
|
||
| def tearDown(self): | ||
| settings.showList = [] | ||
| self.show.deleteShow(True) | ||
| self.show.delete_show(True) |
There was a problem hiding this comment.
Method call delete_show should be deleteShow to match the updated naming convention.
- self.show.delete_show(True)
+ self.show.deleteShow(True)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.
| self.show.delete_show(True) | |
| self.show.deleteShow(True) |
| @@ -222,7 +222,7 @@ def _download_propers(self, proper_list): | |||
| continue | |||
|
|
|||
| # get the episode object | |||
| episode_object = proper.show.getEpisode(proper.season, proper.episode) | |||
| episode_object = proper.show.get_episode(proper.season, proper.episode) | |||
There was a problem hiding this comment.
Method call get_episode should be getEpisode to match the updated naming convention.
- episode_object = proper.show.get_episode(proper.season, proper.episode)
+ episode_object = proper.show.getEpisode(proper.season, proper.episode)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.
| episode_object = proper.show.get_episode(proper.season, proper.episode) | |
| episode_object = proper.show.getEpisode(proper.season, proper.episode) |
sickchill/tv.py
Outdated
| @@ -15,6 +15,7 @@ | |||
|
|
|||
| import imdb | |||
| from imdb import Cinemagoer | |||
|
|
|||
There was a problem hiding this comment.
Missing import for send2trash handling.
+ from send2trash import send2trashCommittable 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.
| from send2trash import send2trash |
sickchill/tv.py
Outdated
| @@ -2484,7 +2490,7 @@ | |||
| current_path = absolute_current_path_no_ext | |||
|
|
|||
| if absolute_current_path_no_ext.startswith(self.show.location): | |||
| current_path = absolute_current_path_no_ext[len(self.show.location) :] | |||
| current_path = absolute_current_path_no_ext[len(self.show.location):] | |||
There was a problem hiding this comment.
Incorrect handling of relative path calculation.
- current_path = absolute_current_path_no_ext[len(self.show.location):]
+ # Use os.path.relpath for correct relative path calculationCommittable 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.
| current_path = absolute_current_path_no_ext[len(self.show.location):] | |
| # Use os.path.relpath for correct relative path calculation |
Signed-off-by: miigotu <[email protected]>
Signed-off-by: miigotu <[email protected]>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 15
Configuration used: CodeRabbit UI
Files selected for processing (3)
- .github/workflows/release.yml (1 hunks)
- sickchill/oldbeard/postProcessor.py (5 hunks)
- sickchill/tv.py (70 hunks)
Files not summarized due to errors (1)
- sickchill/tv.py: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (1)
- sickchill/oldbeard/postProcessor.py
Additional comments: 6
.github/workflows/release.yml (6)
- 5-7: The
branchesfilter underworkflow_runis set to[develop]. Verify this aligns with the intended branch strategy, especially if the workflow is expected to run on branches other thandevelop.- 5-7: The
typesfilter includes bothcompletedandrequested. Ensure that triggering on both event types is necessary for the workflow's intended behavior.- 2-10: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [10-29]
The job
versionusesmiigotu/actions-calver@workflowsfor versioning, which is unconventional. Typically, the reference should be to a version tag or commit SHA for stability. Verify the reference is correct and consider pinning to a specific version or commit SHA for reliability.
- 2-10: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [20-20]
The
node-versionis set to'20.x', which is unusually high and might be a typo or an anticipation of future Node.js releases. Confirm that this version is correct and available at the time of this workflow's execution.
- 2-10: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [23-26]
The
setup-poetry-envaction specifiespython-version: "3.11"andpython-cache-dependency-path: pyproject.toml. Ensure that Python 3.11 is compatible with all dependencies and thatpyproject.tomlcorrectly specifies all Python dependencies for caching.
- 2-10: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [28-30]
The use of
poetry versionandyarn versioncommands to set the version string based onsteps.next-version.outputs.releaseis a good practice. However, ensure that the output ofmiigotu/actions-calver@workflowsaligns with the versioning expectations of both package managers.
|
|
||
| on: | ||
| workflow_run: | ||
| workflows: [Python Packaging] | ||
| branches: | | ||
| - develop | ||
| types: | ||
| - completed | ||
| workflows: [ Python Packaging ] | ||
| branches: [ develop ] | ||
| types: [ completed, requested ] | ||
|
|
||
| jobs: | ||
| version: |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [21-21]
Using cache: 'yarn' with actions/setup-node@v4 while running npm install -g yarn seems redundant. If Yarn is being used, consider removing the npm install -g yarn command or clarify the necessity of this approach.
|
Add just a little extra exception capture on mktime. |
Signed-off-by: miigotu <[email protected]>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 40
Configuration used: CodeRabbit UI
Files selected for processing (19)
- sickchill/gui/slick/views/displayShow.mako (3 hunks)
- sickchill/gui/slick/views/editShow.mako (1 hunks)
- sickchill/gui/slick/views/manage.mako (1 hunks)
- sickchill/gui/slick/views/testRename.mako (1 hunks)
- sickchill/oldbeard/helpers.py (3 hunks)
- sickchill/oldbeard/name_parser/parser.py (16 hunks)
- sickchill/oldbeard/name_parser/regexes.py (1 hunks)
- sickchill/oldbeard/naming.py (1 hunks)
- sickchill/oldbeard/nzbSplitter.py (3 hunks)
- sickchill/oldbeard/postProcessor.py (20 hunks)
- sickchill/oldbeard/providers/ilcorsaronero.py (1 hunks)
- sickchill/oldbeard/providers/tntvillage.py (1 hunks)
- sickchill/oldbeard/show_queue.py (11 hunks)
- sickchill/oldbeard/tvcache.py (3 hunks)
- sickchill/tv.py (71 hunks)
- sickchill/views/home.py (16 hunks)
- sickchill/views/index.py (2 hunks)
- sickchill/views/manage/index.py (6 hunks)
- tests/test_name_parser.py (4 hunks)
Files not summarized due to errors (1)
- sickchill/tv.py: Error: Message exceeds token limit
Files skipped from review due to trivial changes (1)
- sickchill/oldbeard/providers/tntvillage.py
Files skipped from review as they are similar to previous changes (7)
- sickchill/oldbeard/helpers.py
- sickchill/oldbeard/nzbSplitter.py
- sickchill/oldbeard/postProcessor.py
- sickchill/oldbeard/show_queue.py
- sickchill/oldbeard/tvcache.py
- sickchill/views/home.py
- sickchill/views/manage/index.py
Additional comments: 20
sickchill/oldbeard/providers/ilcorsaronero.py (1)
- 159-159: Correctly updated parameter name from
tryIndexerstotry_indexersto match Python naming conventions.sickchill/views/index.py (2)
- 11-11: Import of
AnyandTYPE_CHECKINGfromtypingmodule is correct and follows best practices for type annotations.- 37-52: Proper use of
TYPE_CHECKINGto avoid circular imports and correct initialization ofBaseHandlerwith type annotations.sickchill/oldbeard/name_parser/regexes.py (1)
- 58-58: Typo correction in the comment is good for clarity.
sickchill/oldbeard/name_parser/parser.py (10)
- 6-6: Importing
datefromdatetimeis necessary for handling date operations later in the code.- 30-33: The addition of type hints improves code readability and helps with static type checking.
- 38-40: The logic to compile regexes based on the
parse_methodor theshow_object's anime status is clear and well-implemented.- 61-61: Removing brackets at the start of the series name during cleaning is a specific use case that might be necessary for certain naming conventions.
- 64-68: The method
_compile_regexesis well-structured to handle different regex modes. The separation into ANIME and NORMAL regexes is logical.- 197-211: The logic to pick the best match based on score and regex used is sound. However, the handling of the show object, especially the comparison of
indexerid, is crucial and seems correctly implemented.- 325-326: The method
check_anime_preferredcorrectly prioritizes anime matches if the show is marked as anime. This is a good approach for handling different types of content.- 348-365: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [339-359]
The method
_combine_resultsis a utility function for merging results from different sources. The logic is straightforward and handles various cases well.
- 362-362: Converting bytes to a string using a specified encoding is a common operation. The implementation here is standard and correct.
- 414-414: Converting the input name to Unicode at the beginning of the
parsemethod is a good practice for ensuring consistent string handling throughout the method.tests/test_name_parser.py (4)
- 179-179: The attribute
showObjhas been correctly renamed toshow_objectto adhere to Python's naming conventions.- 306-306: The attribute
showObjhas been correctly renamed toshow_objectto adhere to Python's naming conventions.- 478-478: The attribute
showObjhas been correctly renamed toshow_objectto adhere to Python's naming conventions.- 557-557: The attribute
showObjhas been correctly renamed toshow_objectto adhere to Python's naming conventions.sickchill/gui/slick/views/editShow.mako (1)
- 82-82: The change from
value="${show._location}"tovalue="${show.get_location}"correctly reflects the use of a getter method for accessing the show's location, aligning with best practices for encapsulation.sickchill/gui/slick/views/displayShow.mako (1)
- 407-409: The conditional check
if episode_location and show.get_location and episode_location.lower().startswith(show.get_location.lower()):assumesshow.get_locationis a method call but it's used as a property. Ensureget_locationis correctly implemented as a property in theshowobject, otherwise, this will raise an AttributeError.
| if episode_location: | ||
| text = '<span title="' + episode_location + '" class="addQTip">' + text + "</span>" |
There was a problem hiding this comment.
The variable episode_location is used within an HTML attribute without proper escaping, potentially leading to Cross-Site Scripting (XSS) vulnerabilities. Ensure to escape episode_location before embedding it into the HTML.
| @@ -523,7 +523,7 @@ | |||
| % endif | |||
| ${epResult["name"]} | |||
| </td> | |||
| <td class="col-name location">${epLoc}</td> | |||
| <td class="col-name location">${episode_location}</td> | |||
There was a problem hiding this comment.
The display of episode_location directly in the HTML could lead to Cross-Site Scripting (XSS) vulnerabilities if the variable contains untrusted data. Ensure episode_location is properly sanitized or escaped before being rendered.
| @@ -78,14 +78,14 @@ | |||
| % for current_episode in sorted(show.episodes[current_season], reverse=True): | |||
| <% | |||
| episode_object = show.episodes[current_season][current_episode] | |||
| if not (episode_object and episode_object._location): | |||
| if not (episode_object and episode_object.location): | |||
There was a problem hiding this comment.
Missing error handling for episode_object.location and show.get_location calls.
| continue | ||
|
|
||
| episode_list = episode_object.sorted_episode_list | ||
| if episode_object.episode != min(episode_list): | ||
| continue | ||
|
|
||
| location = episode_object.location[len(show._location)+1:] | ||
| location = episode_object.location[len(show.get_location)+1:] |
There was a problem hiding this comment.
Potential performance issue: episode_object.location[len(show.get_location)+1:] could be inefficient if show.get_location is a costly operation.
- location = episode_object.location[len(show.get_location)+1:]
+ show_location_length = len(show.get_location)
+ location = episode_object.location[show_location_length+1:]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.
| location = episode_object.location[len(show.get_location)+1:] | |
| show_location_length = len(show.get_location) | |
| location = episode_object.location[show_location_length+1:] |
| @@ -161,7 +161,7 @@ def validate_name(pattern, multi=None, anime_type=None, file_only=False, abd=Fal | |||
| logger.debug("Trying to parse " + new_name) | |||
|
|
|||
| try: | |||
| result = NameParser(True, showObj=ep.show, naming_pattern=True).parse(new_name) | |||
| result = NameParser(True, show_object=ep.show, naming_pattern=True).parse(new_name) | |||
There was a problem hiding this comment.
Incorrect variable name showObj changed to show_object but not reflected in the function call.
| assert re.sub(r"\D*", "", air_date) not in excluded_shows | ||
|
|
||
| # noinspection PyUnresolvedReferences | ||
| try: | ||
| check = parse(air_date, fuzzy_with_tokens=True)[0].date() | ||
| check: date = parse(air_date, fuzzy=True).date() |
There was a problem hiding this comment.
Using assert for control flow is not recommended in production code as assertions can be globally disabled with the -O and -OO flags, leading to this check being skipped.
- assert re.sub(r"\D*", "", air_date) not in excluded_shows
+ if re.sub(r"\D*", "", air_date) in excluded_shows:
+ continueCommittable 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.
| assert re.sub(r"\D*", "", air_date) not in excluded_shows | |
| # noinspection PyUnresolvedReferences | |
| try: | |
| check = parse(air_date, fuzzy_with_tokens=True)[0].date() | |
| check: date = parse(air_date, fuzzy=True).date() | |
| if re.sub(r"\D*", "", air_date) in excluded_shows: | |
| continue | |
| try: | |
| check: date = parse(air_date, fuzzy=True).date() |
| except Exception as error: | ||
| logger.debug(error) |
There was a problem hiding this comment.
Catching a broad Exception and logging it without handling specific cases can obscure the root cause of errors. Consider catching more specific exceptions or adding additional context to the log message.
- logger.debug(error)
+ logger.debug(f"Error parsing air date {air_date}: {error}")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.
| except Exception as error: | |
| logger.debug(error) | |
| except Exception as error: | |
| logger.debug(f"Error parsing air date {air_date}: {error}") |
| episode_object = sickchill.indexer.episode(best_result.show, firstAired=best_result.air_date) | ||
| season_number = episode_object["airedSeason"] | ||
| episode_numbers = [episode_object["airedEpisode"]] | ||
| except Exception as error: | ||
| logger.debug(error) |
There was a problem hiding this comment.
Attempting to fetch episode information from an external source (assumed by the context) without handling specific exceptions can lead to uninformative error messages. Consider specifying the type of exceptions you expect.
- except Exception as error:
+ except (IndexerError, ValueError) as error: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.
| episode_object = sickchill.indexer.episode(best_result.show, firstAired=best_result.air_date) | |
| season_number = episode_object["airedSeason"] | |
| episode_numbers = [episode_object["airedEpisode"]] | |
| except Exception as error: | |
| logger.debug(error) | |
| episode_object = sickchill.indexer.episode(best_result.show, firstAired=best_result.air_date) | |
| season_number = episode_object["airedSeason"] | |
| episode_numbers = [episode_object["airedEpisode"]] | |
| except (IndexerError, ValueError) as error: | |
| logger.debug(error) |
| except Exception as error: | ||
| logger.debug(error) |
There was a problem hiding this comment.
Catching a broad Exception during number conversion is too generic. It's better to catch specific exceptions that you expect during integer conversion or when dealing with Roman numerals.
- except Exception as error:
+ except ValueError as error: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.
| except Exception as error: | |
| logger.debug(error) | |
| except ValueError as error: | |
| logger.debug(error) |
…lization to fail somehow Signed-off-by: miigotu <[email protected]>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- sickchill/gui/slick/views/layouts/main.mako (1 hunks)
- sickchill/views/common.py (1 hunks)
- sickchill/views/index.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- sickchill/views/index.py
Additional comments: 2
sickchill/views/common.py (1)
- 46-46: The key
"sbStartTime"has been renamed to"scStartTime"in theself.contextdictionary. Ensure all references to this key across the project are updated to maintain consistency.Verification successful
Given the output from the shell script, which indicates no occurrences of
sbStartTimewere found in.pyand.makofiles, it suggests that the key"sbStartTime"is not used elsewhere in the project or has already been updated to"scStartTime". This aligns with the requirement to ensure all references to this key across the project are updated for consistency, as mentioned in the review comment.Therefore, based on the provided script output, it can be concluded that there are no remaining references to the old key
"sbStartTime"in the project, indicating that the necessary updates for consistency have likely been made.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for old key usage in .py and .mako files rg --type py --type mako 'sbStartTime'Length of output: 38
sickchill/gui/slick/views/layouts/main.mako (1)
- 385-385: The variable
sbStartTimehas been updated toscStartTimefor load time calculation. Verify that this change is reflected everywhere the variable is used.Verification successful
Given the output from the shell script execution, it appears that the search for the old variable
sbStartTimewithin.makofiles did not yield any results. This suggests that the variablesbStartTimeis not used in any.makofiles in the codebase, which supports the notion that the update toscStartTimehas been consistently applied, at least within the scope of.makofiles. Without evidence of the old variable name still being in use, it's reasonable to conclude that the change toscStartTimehas been properly reflected where expected in the.makofiles.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for old variable usage in .mako files rg --type mako 'sbStartTime'Length of output: 28
…ntext variables, refactor variable names Signed-off-by: miigotu <[email protected]>
Signed-off-by: miigotu <[email protected]>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 30
Configuration used: CodeRabbit UI
Files selected for processing (7)
- .editorconfig (2 hunks)
- sickchill/gui/slick/views/layouts/main.mako (7 hunks)
- sickchill/gui/slick/views/restart.mako (1 hunks)
- sickchill/tv.py (71 hunks)
- sickchill/views/common.py (3 hunks)
- sickchill/views/index.py (2 hunks)
- sickchill/views/server_settings.py (1 hunks)
Files not summarized due to errors (1)
- sickchill/tv.py: Error: Message exceeds token limit
Files skipped from review due to trivial changes (1)
- sickchill/views/server_settings.py
Files skipped from review as they are similar to previous changes (2)
- sickchill/gui/slick/views/layouts/main.mako
- sickchill/views/index.py
Additional comments: 3
sickchill/gui/slick/views/restart.mako (1)
- 3-3: The variable name change from
${sbDefaultPage}to${sc_default_page}in themetatag aligns with the refactor's goal for consistency and readability. Ensure all references to this variable in JavaScript and other Mako templates are also updated to maintain consistency.Verification successful
The executed shell scripts aimed to find occurrences of the old variable name
$sbDefaultPagein JavaScript and Mako template files within the project. The absence of output from these commands suggests that there are no references to$sbDefaultPageremaining in these file types. This indicates that the variable name change to${sc_default_page}might have been thoroughly applied across the relevant files, aligning with the refactor's goal for consistency and readability.Given the information obtained, it appears that the review comment's concern has been addressed, and there are no lingering references to the old variable name in JavaScript or Mako templates. Therefore, the change seems to have been correctly implemented across the codebase.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for old variable name usage in JavaScript files rg --type js "\$sbDefaultPage" # Search for old variable name usage in Mako templates rg --type mako "\$sbDefaultPage"Length of output: 65
sickchill/views/common.py (2)
- 39-48: Renaming keys in the
self.contextdictionary improves clarity and consistency with the project's naming conventions. Verify that all references to these keys in the SickChill codebase, including Mako templates and JavaScript files, have been updated to reflect these changes.Verification successful
The executed shell scripts aimed to find references to old key names within Mako templates and JavaScript files in the SickChill codebase. The search did not yield any results, indicating that there are no references to the old key names in these file types. This suggests that either the renaming has been thoroughly updated across the relevant files, or these specific keys were not used in Mako templates and JavaScript files to begin with.
Given the absence of findings, it appears that the changes to the
self.contextdictionary keys have been properly reflected in the SickChill codebase, at least in the context of Mako templates and JavaScript files. However, without a comprehensive search across the entire codebase or specific confirmation of updates in other potentially affected areas (e.g., Python files, other template types), there remains a slight possibility that some references could have been missed.Since the script execution did not reveal any issues and based on the information provided, it seems appropriate to conclude that the renaming of keys in the
self.contextdictionary has been handled correctly in the areas checked. However, this conclusion is based on the assumption that the search was exhaustive for the specified file types and that other areas of the codebase that could be affected by these changes have been similarly updated or were not impacted.* 62-62: Adding `mako_load_start_time` to the context is a good practice for performance monitoring. Ensure this new context variable is utilized appropriately in the templates for debugging or performance analysis purposes.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for old key names usage in Mako templates and JavaScript files rg --type mako "sbStartTime|sbHttpPort|sbHttpsPort|sbHttpsEnabled|sbHandleReverseProxy|sbDefaultPage|scLogin|numErrors|numWarnings" rg --type js "sbStartTime|sbHttpPort|sbHttpsPort|sbHttpsEnabled|sbHandleReverseProxy|sbDefaultPage|scLogin|numErrors|numWarnings"Length of output: 265
There was a problem hiding this comment.
Review Status
Actionable comments generated: 29
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sickchill/tv.py (71 hunks)
- sickchill/views/index.py (2 hunks)
Files not summarized due to errors (1)
- sickchill/tv.py: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (1)
- sickchill/views/index.py
| @@ -125,18 +125,18 @@ def __init__(self, indexer, indexerid: int, lang=""): | |||
|
|
|||
| self.lock = threading.Lock() | |||
| self.episodes = {} | |||
| self.nextaired = "" | |||
| self.next_airdate = "" | |||
There was a problem hiding this comment.
Initialization of self.next_airdate without prior declaration or documentation.
+ # Ensure to document the purpose of `self.next_airdate` and declare it in the class __init__ method for clarity.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.
| self.next_airdate = "" | |
| # Ensure to document the purpose of `self.next_airdate` and declare it in the class __init__ method for clarity. | |
| self.next_airdate = "" |
| other_show = Show.find(settings.showList, self.indexerid) | ||
| if other_show is not None: |
There was a problem hiding this comment.
Exception message in MultipleShowObjectsException is not descriptive.
- raise MultipleShowObjectsException("Can't create a show if it already exists")
+ raise MultipleShowObjectsException(f"Show with indexerid {self.indexerid} already exists in settings.showList")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.
| other_show = Show.find(settings.showList, self.indexerid) | |
| if other_show is not None: | |
| other_show = Show.find(settings.showList, self.indexerid) | |
| if other_show is not None: |
| @property | ||
| def location(self): |
There was a problem hiding this comment.
Redundant directory existence check in location property.
- if settings.CREATE_MISSING_SHOW_DIRS or os.path.isdir(self._location):
+ if settings.CREATE_MISSING_SHOW_DIRS: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.
| @property | |
| def location(self): | |
| @property | |
| def location(self): | |
| if settings.CREATE_MISSING_SHOW_DIRS: |
| # Don't validate dir if user wants to add shows without creating a dir | ||
| if settings.ADD_SHOWS_WO_DIR or os.path.isdir(newLocation): | ||
| if self._location != newLocation: | ||
| if settings.ADD_SHOWS_WO_DIR or os.path.isdir(new_location): |
There was a problem hiding this comment.
Direct use of os.path.isdir without validating new_location.
+ # Ensure new_location is sanitized before this checkCommittable 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.
| if settings.ADD_SHOWS_WO_DIR or os.path.isdir(new_location): | |
| # Ensure new_location is sanitized before this check | |
| if settings.ADD_SHOWS_WO_DIR or os.path.isdir(new_location): |
|
|
||
| self.episodes.clear() | ||
|
|
||
| def getAllEpisodes(self, season=None, has_location=False): | ||
| def get_all_episodes(self, season=None, has_location=False): |
There was a problem hiding this comment.
SQL query construction is vulnerable to injection.
- sql_selection += f"FROM tv_episodes tve WHERE showid = {self.indexerid} "
+ sql_selection += "FROM tv_episodes tve WHERE showid = ? "
+ # Later, use parameterized queries when executing this SQL command, passing self.indexerid as a parameter.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.
| def get_all_episodes(self, season=None, has_location=False): | |
| def get_all_episodes(self, season=None, has_location=False): |
|
|
||
| def specifyEpisode(self, season, episode): | ||
| sql_results = self.loadFromDB(season, episode) | ||
| def specify_episode(self, season, episode): |
There was a problem hiding this comment.
Direct modification of class properties without validation in specify_episode.
+ # Validate inputs before modifying class propertiesCommittable 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.
| def specify_episode(self, season, episode): | |
| def specify_episode(self, season, episode): | |
| # Validate inputs before modifying class properties |
| f"status: {self.status}", | ||
| ] | ||
| ) | ||
|
|
||
| def createMetaFiles(self): | ||
| if not os.path.isdir(self.show._location): | ||
| def create_meta_files(self): |
There was a problem hiding this comment.
Lack of edge case handling in proper_path for file path generation.
+ # Add handling for edge cases and validate generated file pathsCommittable 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.
| def create_meta_files(self): | |
| def create_meta_files(self): | |
| # Add handling for edge cases and validate generated file paths |
|
|
||
| def _replace_map(self): | ||
| def replace_map(self): |
There was a problem hiding this comment.
Use of hardcoded strings in replace_map method.
+ # Refactor to use configurable options or constants instead of hardcoded strings.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.
| def replace_map(self): | |
| def replace_map(self): | |
| # Refactor to use configurable options or constants instead of hardcoded strings. |
| @@ -2260,13 +2271,13 @@ | |||
| else: | |||
| anime_type = 3 | |||
|
|
|||
| replace_map = self._replace_map() | |||
| replace_map = self.replace_map() | |||
There was a problem hiding this comment.
Complex logic without sufficient error handling in naming_pattern.
+ # Simplify logic and add comprehensive error handlingCommittable 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.
| replace_map = self.replace_map() | |
| # Simplify logic and add comprehensive error handling | |
| replace_map = self.replace_map() |
| @@ -2548,7 +2559,7 @@ | |||
| main_db_con = db.DBConnection() | |||
| main_db_con.mass_action(sql_l) | |||
|
|
|||
| def airdateModifyStamp(self): | |||
| def airdate_modify_stamp(self): | |||
There was a problem hiding this comment.
Lack of validation for date operations in airdate_modify_stamp.
+ # Add validation for date operations to handle potential exceptions and ensure the dates are valid before proceeding.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.
| def airdate_modify_stamp(self): | |
| def airdate_modify_stamp(self): | |
| # Add validation for date operations to handle potential exceptions and ensure the dates are valid before proceeding. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sickchill/gui/slick/views/manage_backlogOverview.mako (3 hunks)
- sickchill/views/index.py (3 hunks)
Files skipped from review due to trivial changes (1)
- sickchill/gui/slick/views/manage_backlogOverview.mako
Additional comments: 4
sickchill/views/index.py (4)
- 11-11: The addition of
typingimports forAnyandTYPE_CHECKINGis appropriate for type hinting and conditional import checks.- 15-15: Importing
httputilfromtornadois correct and necessary for HTTP utilities within this file.- 47-47: Renaming
startTimetopage_load_start_timeimproves readability and consistency with Python naming conventions.- 186-189: Adding conditional logging based on
settings.DEVELOPERis a good practice for development environments. Ensure thatsettings.DEVELOPERis properly documented and managed to avoid confusion or misuse.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- sickchill/gui/slick/views/manage_backlogOverview.mako (2 hunks)
- sickchill/gui/slick/views/trendingShows.mako (1 hunks)
- sickchill/oldbeard/searchBacklog.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- sickchill/gui/slick/views/manage_backlogOverview.mako
Additional comments: 3
sickchill/gui/slick/views/trendingShows.mako (1)
- 43-43: Removing the
data-no-redirectattribute from the "Add Show" button will change its behavior. Confirm this aligns with the intended user experience and test to ensure no unintended side effects occur.sickchill/oldbeard/searchBacklog.py (2)
- 18-18: Conversion to integer before passing to
datetime.date.fromordinal()is correct and improves robustness.- 142-142: Renaming
getEpisode()toget_episode()improves consistency with Python naming conventions. Ensure all calls to this method have been updated across the codebase.Verification successful
The verification process has confirmed that the method
getEpisode()was renamed toget_episode()in the context of theshowobject's method, and all calls to this method across the codebase have been updated accordingly. The occurrences ofgetEpisode()found in the output are related to a different context (a method defined withinsickchill/views/home.py), which does not contradict the review comment's intent regarding theshowobject's method renaming. Therefore, the review comment is consistent with the changes made in the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for old method name usage to ensure it's been fully updated. rg --type py 'getEpisode\(' # Search for new method name to confirm the update. rg --type py 'get_episode\('Length of output: 5012
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 9520638 | Triggered | Generic Password | 0ac7c4c | sickchill/oldbeard/subtitles.py | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Our GitHub checks need improvements? Share your feedbacks!
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (4)
- sickchill/gui/slick/views/config_subtitles.mako (1 hunks)
- sickchill/oldbeard/subtitles.py (5 hunks)
- sickchill/start.py (7 hunks)
- sickchill/views/config/subtitles.py (2 hunks)
Additional comments: 7
sickchill/views/config/subtitles.py (1)
- 39-44: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-104]
The removal of
legendastv_userandlegendastv_passparameters from thesaveSubtitlesfunction aligns with the closure of the 'legendastv' provider. Ensure that there are no remaining references to these parameters elsewhere in the codebase that could lead to errors.Verification successful
The search for any remaining references to 'legendastv_user' and 'legendastv_pass' in the codebase did not yield any results. This suggests that the removal of these parameters from the
saveSubtitlesfunction has been handled appropriately, with no lingering references that could lead to errors. The changes align with the intended removal and cleanup related to the 'legendastv' provider.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to 'legendastv_user' and 'legendastv_pass' rg --type py 'legendastv_(user|pass)'Length of output: 39
sickchill/oldbeard/subtitles.py (3)
- 25-25: Confirmed the removal of the 'legendastv' provider. This change is consistent with the closure of the service.
- 40-40: URL updates to HTTPS are correctly implemented for 'addic7ed' and other providers. This enhances security.
- 444-444: Modification to the
get_episodemethod call appears appropriate. Ensure that this change is reflected wherever the method is called to maintain consistency.sickchill/gui/slick/views/config_subtitles.mako (1)
- 326-331: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Removal of the
providerLoginDictentry for'legendastv'is consistent with the provider's closure. Verify that there are no other references to 'legendastv' in the template that need to be addressed.Verification successful
Based on the search conducted in the mako templates, there are no remaining references to 'legendastv'. This suggests that the removal of the
providerLoginDictentry for'legendastv'has been handled appropriately, with no overlooked references in the template.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to 'legendastv' in the mako templates rg --type mako 'legendastv'Length of output: 27
sickchill/start.py (2)
- 127-137: Parameter names in the
restore_cachefunction have been updated for clarity. Ensure all calls to this function reflect the new parameter names.- 1698-1701: The attempt to open a web browser using
webbrowser.openwith a new=2 argument followed by a fallback to new=1 is a good practice to ensure compatibility across different platforms and environments.
| @@ -106,7 +101,7 @@ def __getattr__(self, attr): | |||
|
|
|||
| def sorted_service_list(): | |||
| new_list = [] | |||
| lmgtfy = "https://lmgtfy.com/?q=%s" | |||
| lmgtfy = "https://blog.lmgtfy.com/?q=%s" | |||
There was a problem hiding this comment.
as lmgtfy is dead changed but not sure right or not.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sickchill/gui/slick/views/addShows_favoriteShows.mako (1 hunks)
- sickchill/gui/slick/views/addShows_popularShows.mako (1 hunks)
Additional comments: 2
sickchill/gui/slick/views/addShows_popularShows.mako (1)
- 78-78: Removing
data-no-redirectalters the behavior of the "Add Show" button, potentially enabling redirection post-action. Ensure this change aligns with the intended user experience and does not disrupt user flow.sickchill/gui/slick/views/addShows_favoriteShows.mako (1)
- 99-99: Similar to the previous file, ensure the removal of
data-no-redirectfrom the "Add Show" button is intentional and enhances user experience without causing confusion or disrupting user flow.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 47
Configuration used: CodeRabbit UI
Files selected for processing (5)
- sickchill/oldbeard/helpers.py (33 hunks)
- sickchill/oldbeard/notifiers/libnotify.py (1 hunks)
- sickchill/oldbeard/postProcessor.py (31 hunks)
- sickchill/oldbeard/providers/norbits.py (1 hunks)
- sickchill/oldbeard/scene_numbering.py (5 hunks)
Additional comments: 9
sickchill/oldbeard/providers/norbits.py (1)
- 30-30: The change to use an f-string for the exception message in
_check_authmethod improves readability and is consistent with modern Python practices.sickchill/oldbeard/scene_numbering.py (4)
- 26-26: Adding the
indexerparameter toget_scene_numberingand other functions is a significant change. Ensure all calls to these functions throughout the codebase are updated to include the new parameter.Verification successful
The verification process, including the search for calls to
get_scene_numberingwith both the correct and incorrect number of parameters, suggests that the addition of theindexerparameter to this function and its calls throughout the codebase have been correctly implemented. No instances were found where the function was called with fewer parameters than expected, indicating that the necessary updates have been made.* 213-213: Renaming `show.flushEpisodes()` to `show.flush_episodes()` aligns with Python naming conventions. Verify that all references to the old method name are updated. * 310-310: The addition of the `scene_season` parameter to `get_indexer_absolute_numbering_for_xem` introduces more flexibility. Ensure that the implementation accounts for this parameter correctly in all relevant calls. * 465-466: Adding a `force` parameter to `xem_refresh` allows for more control over the refresh behavior. Ensure that the logic handling this parameter is correctly implemented and tested.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to get_scene_numbering without the new indexer parameter. ast-grep --lang python --pattern $'get_scene_numbering($_, $_, $_, $_)'Length of output: 178
Script:
#!/bin/bash # Search for calls to get_scene_numbering with fewer than the expected number of parameters to ensure all instances have been updated. ast-grep --lang python --pattern $'get_scene_numbering($_, $_, $_)'Length of output: 68
sickchill/oldbeard/postProcessor.py (4)
- 198-220: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [141-217]
The method
list_associated_fileshas been modified to include additional parameters. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.
- 409-409: Method
_move_and_symlinkhas been renamed from_moveAndSymlinkto follow snake_case convention. Verify all references to this method have been updated accordingly.- 755-759: The method call
get_episodeinside_get_ep_objshould be updated toget_episodefollowing the project-wide refactor to snake_case naming convention. This comment is redundant as the method name hasn't changed. Skip generating a similar comment.- 1057-1057: The method call
write_metadatainside_finalizeshould be updated towrite_metadatafollowing the project-wide refactor to snake_case naming convention. This comment is redundant as the method name hasn't changed. Skip generating a similar comment.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sickchill/oldbeard/notifiers/libnotify.py (1 hunks)
- sickchill/oldbeard/postProcessor.py (33 hunks)
Files skipped from review as they are similar to previous changes (1)
- sickchill/oldbeard/notifiers/libnotify.py
sickchill/oldbeard/postProcessor.py
Outdated
| @@ -118,25 +117,28 @@ | |||
| if os.path.isfile(existing_file): | |||
| # see if it's bigger than our old file | |||
| if os.path.getsize(existing_file) > os.path.getsize(self.directory): | |||
| self._log(_("File {existing_file} is larger than {directory}").format(existing_file=existing_file, directory=self.directory), logger.DEBUG) | |||
| self._log(_(f"File {existing_file} is larger than {self.directory}"), logger.DEBUG) | |||
There was a problem hiding this comment.
Use of _ function for localization with f-string might not work as expected. Pass variables as arguments to _ instead.
- self._log(_(f"File {existing_file} is larger than {self.directory}"), logger.DEBUG)
+ self._log(_("File {existing_file} is larger than {directory}").format(existing_file=existing_file, directory=self.directory), logger.DEBUG)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.
| self._log(_(f"File {existing_file} is larger than {self.directory}"), logger.DEBUG) | |
| self._log(_("File {existing_file} is larger than {directory}").format(existing_file=existing_file, directory=self.directory), logger.DEBUG) |
|
|
||
| os.remove(cur_file) | ||
|
|
||
| # do the library update for synoindex | ||
| notifiers.synoindex_notifier.deleteFile(cur_file) | ||
|
|
||
| def _combined_file_operation(self, file_path, new_path, new_base_name, associated_files=False, action=None, subtitles=False): | ||
| def _combined_file_operation(self, file_path, new_path: str, new_base_name, associated_files=False, action=None, subtitles=False): |
There was a problem hiding this comment.
Parameter new_path in _combined_file_operation method is annotated as str, but no type checking is performed. Consider validating new_path to ensure it's a string.
+ if not isinstance(new_path, str):
+ raise TypeError("new_path must be a string")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.
| def _combined_file_operation(self, file_path, new_path: str, new_base_name, associated_files=False, action=None, subtitles=False): | |
| def _combined_file_operation(self, file_path, new_path: str, new_base_name, associated_files=False, action=None, subtitles=False): | |
| if not isinstance(new_path, str): | |
| raise TypeError("new_path must be a string") |
| @@ -1779,8 +1778,8 @@ | |||
| "searchstatus": search_status, | |||
| "status": statusStrings[episode_object.status], | |||
| "quality": self.getQualityClass(episode_object), | |||
| "overview": Overview.overviewStrings[show_obj.getOverview(episode_object.status)], | |||
| "location": relative_ep_location(episode_object._location, show_obj._location), | |||
| "overview": Overview.overviewStrings[show_obj.get_overview(episode_object.status)], | |||
There was a problem hiding this comment.
The method get_overview() is called inside a nested loop, potentially leading to significant performance issues.
Consider optimizing by caching the overview results outside the nested loop.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Configuration used: CodeRabbit UI
Files selected for processing (5)
- sickchill/gui/slick/views/displayShow.mako (7 hunks)
- sickchill/gui/slick/views/home.mako (3 hunks)
- sickchill/views/api/webapi.py (6 hunks)
- sickchill/views/config/backup.py (2 hunks)
- sickchill/views/home.py (24 hunks)
Files not reviewed due to errors (1)
- (no review received)
Files skipped from review as they are similar to previous changes (1)
- sickchill/views/home.py
Additional comments: 1
sickchill/views/config/backup.py (1)
- 31-31: Rename
finalResulttofinal_resultfor consistency with Python naming conventions.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (14)
- sickchill/oldbeard/notifiers/gotify.py (1 hunks)
- sickchill/views/changelog.py (1 hunks)
- sickchill/views/config/anime.py (1 hunks)
- sickchill/views/config/providers.py (1 hunks)
- sickchill/views/config/search.py (1 hunks)
- sickchill/views/config/subtitles.py (3 hunks)
- sickchill/views/history.py (1 hunks)
- sickchill/views/home.py (27 hunks)
- sickchill/views/index.py (4 hunks)
- sickchill/views/manage/add_shows.py (1 hunks)
- sickchill/views/manage/index.py (8 hunks)
- sickchill/views/manage/post_processing.py (1 hunks)
- sickchill/views/movies.py (2 hunks)
- sickchill/views/news.py (1 hunks)
Files skipped from review due to trivial changes (4)
- sickchill/views/config/anime.py
- sickchill/views/config/providers.py
- sickchill/views/manage/add_shows.py
- sickchill/views/movies.py
Files skipped from review as they are similar to previous changes (3)
- sickchill/views/config/subtitles.py
- sickchill/views/home.py
- sickchill/views/index.py
Additional comments: 9
sickchill/views/news.py (1)
- 28-35: LGTM! The restructuring of the
t.render()method call enhances readability and maintainability.sickchill/views/changelog.py (1)
- 24-31: LGTM! The restructuring of the
t.render()method call enhances readability and maintainability.sickchill/views/manage/post_processing.py (1)
- 15-21: LGTM! The restructuring of the
t.render()method call enhances readability and maintainability.sickchill/views/history.py (1)
- 19-19: LGTM! Using
try_intforsettings.HISTORY_LIMITimproves type safety and robustness.sickchill/oldbeard/notifiers/gotify.py (1)
- 119-119: LGTM! Making the
hostparameter optional in_notify_gotifyincreases method flexibility.sickchill/views/config/search.py (1)
- 22-27: LGTM! The restructuring of the
t.render()method call enhances readability and maintainability.sickchill/views/manage/index.py (3)
- 21-27: LGTM! The restructuring of the
returnstatement in theindexmethod enhances readability.- 547-548: Ensure
new_show_dircalculation considers potential issues withroot_dir_mapchanges. Validate the new directory path before applying.- 715-715:
removeparameter retrieval as an array (remove[]) might not align with standard query parameter conventions. Verify if this is handled correctly on the client side.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sickchill/oldbeard/helpers.py (3 hunks)
- sickchill/oldbeard/postProcessor.py (20 hunks)
Files skipped from review as they are similar to previous changes (2)
- sickchill/oldbeard/helpers.py
- sickchill/oldbeard/postProcessor.py
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