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

A few changes to the original code #23

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

AnakinFoxe
Copy link

@AnakinFoxe AnakinFoxe commented Mar 28, 2024

First of all, thanks for this great script. It helped me download all the albums for my kid 😆

Made a few changes to the original code:

  • Authenticate with password (with -p PASSWORD option), however if session ID is provided, password will be ignored.
  • Add an option --folder to download all the albums under the specific folder
  • Concurrently download images of the same album
  • A few useful refactors (I think)
    • Split codes into functions and organize under class SmugMugDownloader
    • Use one session for all the requests
    • Use logging
    • Use @retry to manage retry for get_json()
    • Use Makefile to setup a virtual environment to work with

@andyjsmith
Copy link
Owner

Sorry for the late reply, I appreciate the changes!

I'm a bit hesitant on the password authentication due to some unhandled edge cases (e.g. what if the user has 2FA enabled?). Maybe we could keep both the user/pass and session token auth mechanisms instead of replacing it. As for the Makefile, I would like to keep this script platform-independent so I don't want to merge in a Unix-specific script. At some point a setup.py / pyproject.toml may be appropriate.

The other contributions are great but they probably need to be split off as separate PRs.

Copy link

@jason-fer jason-fer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This repo is a lifesaver. Good point on allowing 2 separate modes Andy.

@AnakinFoxe
Copy link
Author

@andyjsmith thanks for the comments! I have updated the PR with following changes (updated the PR description as well)

  • Kept the -s SESSION argument. If session ID is provided, password will be ignored.
  • Removed Makefile
  • README.md has been updated accordingly.

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.

3 participants