feat: delete tg post video & update FinalRip & store relative path #10
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis PR implements several significant changes to improve the pipeline's functionality and reliability. The main changes include updating the TaskInfo model with new fields, modifying the FinalRip encoding process to support video slicing options, replacing video uploads with text-only posts in Telegram, and removing the local Telegram bot API server dependency. Sequence diagram for the updated pipeline processsequenceDiagram
participant User
participant Pipeline
participant TaskExecutor
participant FinalRipClient
participant TGChannelSender
User->>Pipeline: Start pipeline
Pipeline->>TaskExecutor: Submit task
TaskExecutor->>FinalRipClient: Start FinalRip task
FinalRipClient-->>TaskExecutor: Task completed
TaskExecutor->>TGChannelSender: Send text post
TGChannelSender-->>TaskExecutor: Post successful
Updated class diagram for TaskInfoclassDiagram
class TaskInfo {
+DirectoryPath download_path
+str uploader
+str script_content
+str param_content
+Optional~bool~ slice
+Optional~int~ timeout
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Tohrusky - I've reviewed your changes - here's some feedback:
Overall Comments:
- The removal of retry logic for failed merges in pipeline_finalrip() could reduce reliability. Consider keeping the retry mechanism or document why it's no longer needed.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 4 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -38,7 +37,7 @@ async def test_start_task(self) -> None: | |||
| s = v | |||
| print(repr(s)) | |||
| try: | |||
| await self.finalrip.start_task(encode_param=p, script=s, video_key=video_key) | |||
| await self.finalrip.start_task(encode_param=p, script=s, video_key=video_key, slice=True, timeout=10) | |||
There was a problem hiding this comment.
suggestion (testing): Test should verify the new slice and timeout parameters
The test should include cases for different combinations of slice and timeout values to ensure they're properly handled. Also, consider testing the default values.
Suggested implementation:
@pytest.mark.skipif(os.environ.get("GITHUB_ACTIONS") == "true", reason="Only test locally")
class Test_FinalRip:
@pytest.fixture
def mock_video_key(self):
return "test_video_key"
@pytest.fixture
def mock_encode_param(self):
return {"param": "test"}
@pytest.fixture
def mock_script(self):
return "test_script"
def setup_method(self) -> None: s = v
print(repr(s))
try:
await self.finalrip.start_task(encode_param=p, script=s, video_key=video_key, slice=True, timeout=10)
except Exception as e:
print(e)
@pytest.mark.asyncio
async def test_start_task_default_values(self, mock_video_key, mock_encode_param, mock_script):
# Test with default values (assuming slice=False and no timeout by default)
await self.finalrip.start_task(
encode_param=mock_encode_param,
script=mock_script,
video_key=mock_video_key
)
# Add assertions based on expected behavior with default values
@pytest.mark.asyncio
async def test_start_task_with_slice(self, mock_video_key, mock_encode_param, mock_script):
# Test with slice=True
await self.finalrip.start_task(
encode_param=mock_encode_param,
script=mock_script,
video_key=mock_video_key,
slice=True
)
# Add assertions for slice=True behavior
@pytest.mark.asyncio
async def test_start_task_with_timeout(self, mock_video_key, mock_encode_param, mock_script):
# Test with specific timeout value
await self.finalrip.start_task(
encode_param=mock_encode_param,
script=mock_script,
video_key=mock_video_key,
timeout=15
)
# Add assertions for timeout behavior
@pytest.mark.asyncio
async def test_start_task_with_slice_and_timeout(self, mock_video_key, mock_encode_param, mock_script):
# Test with both slice and timeout
await self.finalrip.start_task(
encode_param=mock_encode_param,
script=mock_script,
video_key=mock_video_key,
slice=True,
timeout=10
)
# Add assertions for combined slice and timeout behavior
@pytest.mark.asyncio
async def test_start_task_invalid_timeout(self, mock_video_key, mock_encode_param, mock_script):
# Test with invalid timeout value
with pytest.raises(ValueError):
await self.finalrip.start_task(
encode_param=mock_encode_param,
script=mock_script,
video_key=mock_video_key,
timeout=-1
)To complete these tests, you'll need to:
- Add appropriate assertions in each test case based on the expected behavior of your FinalRip class
- Possibly mock any external dependencies or services that FinalRip.start_task might use
- Add any necessary setup in the setup_method if required by your implementation
- Adjust the mock fixtures based on your actual parameter requirements
Comment on lines
+15
to
17
| task_info = build_task_info(torrent_info, cfg, rss_config, server_config) | ||
| assert isinstance(task_info, TaskInfo) | ||
| print(task_info) |
There was a problem hiding this comment.
suggestion (testing): Test should verify the new TaskInfo fields
Add specific assertions to verify that download_path, slice, and timeout are correctly set in the TaskInfo object. Also, test error cases when server_config is invalid or missing.
Suggested implementation:
def test_build_task_info() -> None:
rss_config: RSSConfig = RSSConfig.from_yaml(CONFIG_PATH / "rss.yml")
server_config: ServerConfig = ServerConfig.from_yaml(CONFIG_PATH / "server.yml")
for cfg in rss_config.nyaa:
torrent_info_list = parse_nyaa(cfg)
for torrent_info in torrent_info_list:
task_info = build_task_info(torrent_info, cfg, rss_config, server_config)
assert isinstance(task_info, TaskInfo)
# Verify TaskInfo fields are set correctly
assert task_info.download_path == server_config.download_path
assert task_info.slice == server_config.slice
assert task_info.timeout == server_config.timeout
def test_build_task_info_invalid_server_config() -> None:
rss_config: RSSConfig = RSSConfig.from_yaml(CONFIG_PATH / "rss.yml")
cfg = next(iter(rss_config.nyaa)) # Get first config for testing
torrent_info = next(iter(parse_nyaa(cfg))) # Get first torrent info
# Test with missing server_config
with pytest.raises(TypeError):
build_task_info(torrent_info, cfg, rss_config, None)
# Test with invalid server_config values
invalid_server_config = ServerConfig.from_yaml(CONFIG_PATH / "server.yml")
invalid_server_config.download_path = None
with pytest.raises(ValueError):
build_task_info(torrent_info, cfg, rss_config, invalid_server_config)You'll need to:
- Import pytest if not already imported at the top of the file
- Ensure ServerConfig class has proper validation for its fields
- Update the test configuration files (rss.yml and server.yml) to have valid test data
Comment on lines
+17
to
+18
| torrent_hash = "5484cff30b108ca1d1987fb6ea4eebed356b9ddd" | ||
| torrent_url = "https://nyaa.si/download/1878677.torrent" |
There was a problem hiding this comment.
issue (testing): Test relies on external torrent that might become unavailable
Consider creating a small test torrent file and including it in the test assets to avoid dependency on external resources that might become unavailable.
Comment on lines
4
to
5
| async def test_task_store() -> None: | ||
| store = AsyncJsonStore() |
There was a problem hiding this comment.
suggestion (testing): Test should verify the renamed tg_posted field
Add test cases to verify that the TaskStatus.tg_posted field (renamed from tg_uploaded) works correctly and maintains backward compatibility if needed.
Suggested implementation:
@pytest.mark.asyncio
async def test_task_store() -> None:
store = AsyncJsonStore()
# Test setting and getting tg_posted field
task = TaskStatus(task_id="test1")
assert not task.tg_posted # Should default to False
task.tg_posted = True
await store.save_task(task)
loaded_task = await store.get_task("test1")
assert loaded_task.tg_posted is True
# Test backward compatibility with tg_uploaded field
old_format_task = {
"task_id": "test2",
"tg_uploaded": True
# Add other required fields here
}
await store.save_task(TaskStatus.from_dict(old_format_task))
loaded_old_task = await store.get_task("test2")
assert loaded_old_task.tg_posted is True # Should read old field correctly
Note: The exact implementation details might need to be adjusted based on:
- The actual fields available in TaskStatus
- The specific backward compatibility mechanism used in TaskStatus
- The actual method signatures for save_task and get_task in AsyncJsonStore
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#7
#8
#9
Summary by Sourcery
Refactor the pipeline to replace video uploads with text posts to Telegram channels, and update the task execution to support video slicing and timeout parameters. Simplify deployment by removing the Telegram bot API service from Docker Compose. Enhance the task building process to include server configuration and use relative paths for file operations. Update tests to align with these changes.
New Features:
Enhancements:
Deployment:
Tests: