Skip to content

fix: update output directory in config.ini and enhance URL handling#26

Open
mvidaldev wants to merge 2 commits intorkwyu:mainfrom
mvidaldev:ignore-subdomain-convert-url
Open

fix: update output directory in config.ini and enhance URL handling#26
mvidaldev wants to merge 2 commits intorkwyu:mainfrom
mvidaldev:ignore-subdomain-convert-url

Conversation

@mvidaldev
Copy link

Added a folder to save downloads, for easy pourpose.

Fixed url handling to ignore subdomains on the npm start.

@rkwyu
Copy link
Owner

rkwyu commented Feb 17, 2026

A few questions:

  • Is there any reason to change the default output directory from output to ./downloads/
  • Shells normally remove surrounding quotes before passing args to Node. The script should work for single quotes and double quotes. Is there a reproducible case where URLs actually contain literal '?
  • The subdomain normalization idea makes sense conceptually, but I think it might be better to handle this in the domain regex modules (EverandRegex.js, ScribdRegex.js, SlideshareRegex.js) rather than in run.js. Those files are already used to classify supported URLs, so putting normalization/canonicalization logic there would centralize URL handling and keep detection consistent across all supported domains (scribd.com, everand.com, slideshare.net)

Just want to understand the rationale and see if centralizing URL logic in the regex modules would be cleaner.

@mvidaldev mvidaldev marked this pull request as draft February 17, 2026 22:09
@mvidaldev
Copy link
Author

I will make changes, it is my first time contributing to an open source project.

@mvidaldev
Copy link
Author

I've made changes as asked, I've tested slideshare and it had some errors processing itens too, we have fixed the problem,

I could not test evergrand because every link fails to me. Could you provide me some valid links to test?

@mvidaldev mvidaldev marked this pull request as ready for review February 18, 2026 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants