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

refactor: ingame_logins repository #489

Merged
merged 6 commits into from
Sep 24, 2023
Merged

refactor: ingame_logins repository #489

merged 6 commits into from
Sep 24, 2023

Conversation

7mochi
Copy link
Contributor

@7mochi 7mochi commented Sep 12, 2023

Describe your changes

Related Issues / Projects

Checklist

  • I've manually tested my code
  • The changes pass pre-commit checks (make lint)
  • The changes follow coding style

@7mochi 7mochi added the enhancement New feature or request label Sep 12, 2023
@7mochi 7mochi self-assigned this Sep 12, 2023
@7mochi 7mochi requested a review from cmyui September 13, 2023 16:45
@NiceAesth NiceAesth added the code quality Something is implemented poorly label Sep 21, 2023
@NiceAesth
Copy link
Contributor

Will you apply the unset suggestion to all of the currently open PRs or will that be a following PR?

Would prefer if it was multiple palletable prs

@7mochi 7mochi mentioned this pull request Sep 21, 2023
17 tasks
@7mochi 7mochi changed the base branch from master to repositories September 21, 2023 13:10
@7mochi
Copy link
Contributor Author

7mochi commented Sep 21, 2023

Unset suggestion applied See #499

@7mochi 7mochi marked this pull request as draft September 22, 2023 01:30
@7mochi 7mochi changed the base branch from repositories to master September 23, 2023 21:15
@7mochi 7mochi force-pushed the ingame-logins-repo branch 2 times, most recently from 21f06ff to a40f6f1 Compare September 23, 2023 21:35
@7mochi 7mochi marked this pull request as ready for review September 23, 2023 21:44
@7mochi 7mochi requested a review from cmyui September 24, 2023 03:12
@cmyui
Copy link
Member

cmyui commented Sep 24, 2023

i think the rest is good

@cmyui cmyui mentioned this pull request Sep 24, 2023
3 tasks
INSERT INTO ingame_logins (userid, ip, osu_ver, osu_stream, datetime)
VALUES (:userid, :ip, :osu_ver, :osu_stream, NOW())
"""
params = {
Copy link
Member

@cmyui cmyui Sep 24, 2023

Choose a reason for hiding this comment

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

ah we should also set dict[str, Any] as the type on each declaration of params because the default behaviour will be to take the types of the initial definition, which will be more specific (less flexible)

Copy link
Member

Choose a reason for hiding this comment

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

but this should only be set on the initial declaration of params - if it's redefined in the same function ,it should not have the type annotation

mypy will point this out

Copy link
Member

Choose a reason for hiding this comment

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

u can see it in the other repos from master after #501

Copy link
Member

Choose a reason for hiding this comment

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

pls apply to all prs as well

@cmyui cmyui merged commit b2e2739 into master Sep 24, 2023
4 checks passed
@cmyui cmyui deleted the ingame-logins-repo branch September 24, 2023 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Something is implemented poorly enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants