-
-
Notifications
You must be signed in to change notification settings - Fork 60
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
Refactoring, fixes and some new features #120
Conversation
Reviewer's Guide by SourceryThis pull request includes a comprehensive refactoring of the codebase, bug fixes, and the addition of new features. The changes involve restructuring the main script, enhancing logging, improving account handling, and adding new utility functions. The refactoring also introduces new classes and methods to improve code readability and maintainability. File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @cal4 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 11 issues found
- 🟡 Security: 2 issues found
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@klept0 the sourcery bot is so annoying lol |
…onal, add some to-dos
I tried your fork (d48fb60) and had some issues with completing the Daily Set. But the crash was caught and it continued to do all the searches. I made a couple changes to have it work in docker, see sed's below. On another note, I got "Read to Earn" working in standalone python code, but is via
|
Had same error today actually. Since then I've committed cal4@db016ce which I believe should fix that particular issue. I didn't intend to change the functionality of this class besides adding additional and more thorough logging, but hopefully this is all that needs to be adjusted. I've also adjusted the try-catch to continue rather than return if there's an error with one of the cards, while still logging the error: cal4@4a633fc. That way the remaining cards, if any, are attempted. Believe this is closer to the OG behavior except with the logging. Btw, thanks for giving it a go! Definitely keeping an eye on this issue. |
…ation; continue versus return if exception
Got an error on new develop branch (46c368e): It got to search 13/30, but looks like got credit for 14 searches. Edit: On a subsequent run, searches getting multiple attempt failed. But the same term (
|
Closing this PR in favor of #137 to avoid confusion |
Recommend turning on "Hide whitespace" due to line-ending change (need to settle on project ending eventually
Added
Changed
Removed
Fixed