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

New approach to download series #124

Merged
merged 6 commits into from
Dec 2, 2021
Merged

New approach to download series #124

merged 6 commits into from
Dec 2, 2021

Conversation

AmirRezaM75
Copy link
Collaborator

@AmirRezaM75 AmirRezaM75 commented Dec 1, 2021

Hey guys ✋
As mentioned here (#123), new laracasts website uses inertiajs.

IMO it's better than algolia complexity because we can simply access to data-page attribute and get latest information.
Right now the app scraps topics page and then find all related series and episodes for that topic. It caches response in cache.php file to don't repeat scraping process from scratch.

probably laracasts removes single lessons and added a section called larabit. I will add this in future update and after fixing possible bugs.

@carlosflorencio I can share my account details with you for a day to test this.

Remove unused Exceptions and rename some methods

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@carlosflorencio
Copy link
Owner

Hey @AmirRezaM75 , thanks for your work on this.

With the black friday discount, I went back and subscribed Laracasts again (for the Vim videos at least) so I have an account to test it. After a first run it seems to be working fine 🎉.

I think I'll have time tomorrow to review these changes, I intend to also simplify the docker usage.

@carlosflorencio carlosflorencio self-requested a review December 1, 2021 20:35
@carlosflorencio
Copy link
Owner

So I went through the several changes and they all seem fine to merge. I didn't test the several arguments the tool receives and the make skips functionality, those are not in the happy path and can be fixed later if not working correctly.

Just a few minor points if you want to act on:

  • When changing the composer.json file we should also update the composer.lock (via composer update)
    • You can push a new composer.lock file
  • Update README.md to have PHP 7 as a requirement and maybe add some paragraph with credits to you

@AmirRezaM75
Copy link
Collaborator Author

Yeah I forgot composer.lock
I removed vendor file and ran composer update and restart the script to see if anything breaks or not. (it seems fine)
Thanks for offer but I don't agree with credit in readme.md, github's project contributors section is enough for that.

@carlosflorencio carlosflorencio merged commit 3b18be2 into carlosflorencio:master Dec 2, 2021
@carlosflorencio
Copy link
Owner

Thank you for the contribution, merged.

@AmirRezaM75 AmirRezaM75 deleted the laracasts_2021 branch December 3, 2021 05:12
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.

None yet

2 participants