Conversation
| @metaprime what you think about this pr? to me this looks fine ... |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors repetitive code patterns across the Ripme codebase by removing unnecessary method parameters, consolidating duplicate functionality, and extracting common patterns into reusable utility methods.
- Removed URL parameter from getAlbumTitle() and pageContainsAlbums() methods since the URL is available as an instance field
- Created a new QueueingRipper class to extract common functionality from AlbumRipper and AbstractHTMLRipper/AbstractJSONRipper
- Refactored cookie handling to use a new collectCookiesInto() method in the Http utility class
Reviewed Changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/rarchives/ripme/ripper/QueueingRipper.java | New base class extracting common queuing functionality from AlbumRipper |
| src/main/java/com/rarchives/ripme/ripper/AbstractRipper.java | Updated getAlbumTitle() method signature to remove URL parameter |
| src/main/java/com/rarchives/ripme/ripper/AlbumRipper.java | Refactored to extend QueueingRipper instead of duplicating functionality |
| src/main/java/com/rarchives/ripme/utils/Http.java | Added collectCookiesInto() method for streamlined cookie handling |
| src/main/java/com/rarchives/ripme/utils/RipUtils.java | Added utility methods for URL creation and manipulation |
| Multiple ripper classes | Updated to use new method signatures without URL parameters |
| Multiple test classes | Updated test calls to match new method signatures |
| String title = firstPage.select("#info > h1").text(); | ||
| if (title == null) { | ||
| return getAlbumTitle(url); | ||
| return getAlbumTitle(); |
There was a problem hiding this comment.
This creates an infinite recursion. The method is calling itself without any base case or different logic path.
| import org.apache.logging.log4j.Logger; | ||
| import org.jsoup.Connection.Method; | ||
| import org.jsoup.Connection.Response; | ||
| import com.rarchives.ripme.utils.Utils; |
There was a problem hiding this comment.
The Utils import appears to be unused in this file. The import should be removed to keep the code clean.
| import com.rarchives.ripme.utils.Utils; |
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.stream.Stream; |
There was a problem hiding this comment.
The Stream import appears to be unused in this file. The import should be removed to keep the code clean.
| import java.util.stream.Stream; |
| .method(Method.POST) | ||
| .response(); | ||
| cookies.putAll(resp.cookies()); | ||
| .post(); |
There was a problem hiding this comment.
The Http.post() method call is missing proper error handling and the result is not being used. Consider storing the result or adding proper error handling.
| @zaphodorama thanks for the update! I'll take a look |
zaphodorama force-pushed the refactors branch from f6e0723 to fd0e3a8 Compare 
Category
This change is exactly one of the following (please change
[ ]to[x]) to indicate which:Description
Removed unnecessary parameters, refactored repetitive code like in the AbstractHtml/AbstractJson and AlbumRipper, or the code which dealt with cookies, etc.
Testing
Required verification:
gradlew test(there are no new failures or errors).Optional but recommended: