Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reddit hangs fix #1539

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Reddit hangs fix #1539

wants to merge 1 commit into from

Conversation

Ditiae
Copy link

@Ditiae Ditiae commented Jan 4, 2020

Category

This change is exactly one of the following (please change [ ] to [x]) to indicate which:

  • a bug fix (Fix #...)
  • a new Ripper
  • a refactoring
  • a style change/fix
  • a new feature

Description

Please add details about your change here.
Made all possible post titles valid for folder/file paths in Windows.
fixes #1536
fixes #1477
fixes #1399
fixes #1357
fixes #1515
fixes #1518

Testing

Required verification:

  • I've verified that there are no regressions in mvn test (there are no new failures or errors).
  • I've verified that this change works as intended.
    • Downloads all relevant content.
    • Downloads content from multiple pages (as necessary or appropriate).
    • Saves content at reasonable file names (e.g. page titles or content IDs) to help easily browse downloaded content.
  • I've verified that this change did not break existing functionality (especially in the Ripper I modified).

Optional but recommended:

  • I've added a unit test to cover my change.

@Ditiae
Copy link
Author

Ditiae commented Jan 4, 2020

Do not merge. Not finished.

Revert "Reddit hangs fix"

This reverts commit 502d553.

real fix.
@madhatr
Copy link

madhatr commented Jan 6, 2020

#1541 Likely related

@cyian-1756
Copy link
Collaborator

@Wea1thRS any update on this?

@Ditiae
Copy link
Author

Ditiae commented Feb 15, 2020

@cyian-1756 I've been really busy, and likely can't work on this anytime soon. I think I've gieven enough information and enough of a poc that someone should be able to polish it up and push it. I just can't right now. Sorry. Feel free to do what you'd like with what I have done so far though.

Edit: To put it simply, the entire issue is really only just windows not allowing some of the characters reddit is giving. If you add a check to remove/change those characters, all issues should be fixed. As not all titles are getting sanitized correctly.

Sorry to leave this pr half-finished, expected to have time to work on it, this wasn't my intention when starting it.

@perpetualogic
Copy link

I was running into this issue in Linux, and applying your changes locally seemed to fix it. I'll keep testing and switch over to Windows for some testing too and see if I can replicate.

@WouterV85
Copy link

Your fix doesn't fix it when the title has a '/' in it's name, because new Fil converts the '/' to ''.
Example

The title (or others) in handleURL in RedditRipper needs a sanitizeSaveAs as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants