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

time.perf_counter() issues, vault_updater bug fix, and moving to f-strings (amongst other cosmetic changes) #49

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

Conversation

CobraCoral
Copy link

Switched to time.time() as time.perf_counter() is not supposed to be
persisted and checked again after long (>1min) periods of time

  • Fixed oob not being set to True when a player was bailed out
  • Fixed cosmetic issue when player_time is 0 (should be checked first)
  • Added a unique_id for !heist play, to fix a small issue where if a player started a heist, and then
    immediately did !heist play again, he would be added to his own
    heist as if he was a crew member
  • Added f-strings everywhere to make code more readable
  • Fixed bug on vaulter_updater (it was trying to access the key which is
    a string as if it was the dictionary)

  Switched to time.time() as time.perf_counter() is not supposed to be
  persisted and checked again after long (>1min) periods of time
- Fixed oob not being set to True when a player was bailed out
- Fixed cosmetic issue when player_time is 0 (should be checked first)
- Added a unique_id for !heist play, to fix a small issue where if a player started a heist, and then
    immediately did !heist play again, he would be added to his own
    heist as if he was a crew member
- Added f-strings everywhere to make code more readable
- Fixed bug on vaulter_updater (it was trying to access the key which is
  a string as if it was the dictionary)
@Drapersniper
Copy link

Drapersniper commented May 26, 2020

You should check with repo maintainers before making cosmetic choices, for example fstrings breaks any potential for i18n support that they may or may not be wanting to add in the future.

@CobraCoral
Copy link
Author

Good point. I could try to split the bug fixes in their own commit and the f-strings in a separate commit if so desired by the repo maintainer. I will wait to see what they say first though, thanks for your input.

@Malarne
Copy link
Owner

Malarne commented May 27, 2020

heist.py

line 142 ctx.prefix
line 238 can be combined with 237 in a single string (multiple f-strings are not really needed)
same as above for 356, 362, 393, 474-475, 482, 506 -- thief.py -- 216, 220, 223, 233, 236, 237, 245, 248, 252, 256, 257, (might be done by black or any other linter ?)
multiple backslashes in f-strings, while you added yourself comments f-strings cannot have backslashes (better be safe)

for i18n support, i really need to take time to rewrite this entire cog so, for now, not planned until then.
Other than that, i really appreciate you took time to find out where the issues were in the code, i hope to merge this PR soon :)

@CobraCoral
Copy link
Author

@Malarne so do you want me to fix those open items before you merge? Thanks!

@Malarne
Copy link
Owner

Malarne commented Jul 4, 2020

yeah please

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.

4 participants