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

QOL improvements #1

Merged
merged 4 commits into from Jan 11, 2020
Merged

QOL improvements #1

merged 4 commits into from Jan 11, 2020

Conversation

@CorySanin
Copy link
Contributor

CorySanin commented Jan 10, 2020

  • Skip steps that don't apply to US residents
  • Complete logging overhaul
  • Logout user and close browser when complete

next time:

  • Add option to run continuously, sleeping in between checking EGS
CorySanin added 2 commits Jan 4, 2020
🇺🇸🇺🇸🇺🇸🇺🇸🇺🇸
@Ricardo-Osorio

This comment has been minimized.

Copy link

Ricardo-Osorio commented on main.py in 9389a24 Jan 9, 2020

I am trying to keep the logs clean from messages that don't add any valuable info.
That means you will only see things such as "games being obtained" or "no games found".
An example of the logs: https://imgur.com/a/el4jJsg
This would be useful if there was a logging library being used which supports level of logging.
To debug or whenever there is an issue I have the CURRENT_STEP variable being dumbed into the logs.

IMO wither replace the print() for pass to keep following the same logic or add prints everywhere else

This comment has been minimized.

Copy link
Owner Author

CorySanin replied Jan 9, 2020

Understood. Is there a logging library you'd be interested in using? I think that'd be the best option since that would allow all the CURRENT_STEP declarations to be replaced with log statements. That would be cleaner, if you ask me. Otherwise I can just get rid of the print statement for now.

This comment has been minimized.

Copy link

Ricardo-Osorio replied Jan 9, 2020

I agree, it would be cleaner that way. I don't have any preference on that, it's up to you :)

This comment has been minimized.

Copy link
Owner Author

CorySanin replied Jan 9, 2020

I'll investigate then 😀

@Ricardo-Osorio

This comment has been minimized.

Copy link

Ricardo-Osorio commented on main.py in 9389a24 Jan 9, 2020

Nice catch 👍

@Ricardo-Osorio

This comment has been minimized.

Copy link

Ricardo-Osorio commented on main.py in e1d68d3 Jan 11, 2020

Do you need the getattr call?

This comment has been minimized.

Copy link
Owner Author

CorySanin replied Jan 11, 2020

I believe so, since setLevel takes an int and I expect LOGLEVEL to be a level name represented as a string. For example, logging.INFO is 20. Does that make sense? Is there a better way to do that? Sorry, Python is not my best language

This comment has been minimized.

Copy link

Ricardo-Osorio replied Jan 11, 2020

Oh I clearly did not had a clear picture of what was happening there. You're right!
Still, you should definitely add a default value (getattr accepts it as the third argument) so it's user proof. That means if the user passes something not valid as the log level (through the env var) it won't break, which happens with the code as is.
Also, by adding a default value you can remove the or "INFO" from line 24 as it will become redundant.

@Ricardo-Osorio

This comment has been minimized.

Copy link

Ricardo-Osorio commented on e1d68d3 Jan 11, 2020

I suggest editing the README so it accommodates the new env variables LOGLEVEL

This comment has been minimized.

Copy link
Owner Author

CorySanin replied Jan 11, 2020

I'll update it once I have that last feature I want to include, since it will have its own env variable.

This comment has been minimized.

Copy link

Ricardo-Osorio replied Jan 11, 2020

Alright 👍
Would you mind making this a different branch on it's own (ideally each feature should be implemented on it's own branch) so I can merge your previous two commits? That is because I can only merge branches so those changes are currently blocked until all your work is done which, for any unexpected reason, could take a long time. It's a good practice.
Thank you!

@CorySanin

This comment has been minimized.

Copy link
Contributor Author

CorySanin commented Jan 11, 2020

In that case, I'm updating the readme now. Ready to merge.

@CorySanin CorySanin changed the title WIP: QOL improvements QOL improvements Jan 11, 2020
@Ricardo-Osorio Ricardo-Osorio merged commit 9109977 into Ricardo-Osorio:master Jan 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.