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.

Sign up for GitHub

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

Jump to bottom

Development refactor/listeners #48

Merged
merged 14 commits into from
Dec 12, 2023
Merged

Development refactor/listeners #48

merged 14 commits into from
Dec 12, 2023

Conversation

KDwevedi
Copy link
Contributor

No description provided.

tushar5526
tushar5526 reviewed Dec 6, 2023
View reviewed changes
cogs/listeners/member_events_cog.py Outdated

@commands.Cog.listener("on_member_join")
async def on_member_join(self, member: discord.Member):
SupabaseInterface().updateContributor(member)
Copy link

Choose a reason for hiding this comment

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

Interfaces are generally meant to be extended and not make instances of. Maybe you can refactor to making a helper class and define static methods on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored into helpers/supabaseClient

tushar5526
tushar5526 reviewed Dec 6, 2023
View reviewed changes
interfaces/supabase.py Outdated
@@ -29,19 +31,33 @@ def update(self, update, query_key, query_value):
def insert(self, data):
data = self.client.table(self.table).insert(data).execute()
Copy link

Choose a reason for hiding this comment

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

Possible case of self.table being equal to ''

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in supabase refactor, class no longer specifies table on initialisation

helpers.py Outdated
@@ -0,0 +1,18 @@
import discord
Copy link

Choose a reason for hiding this comment

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

Import not getting used ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made import more specific

@tushar5526
Copy link

A bunch of observations:

  • Generally, python codes use camel_case for variable and function declaration.
  • I see type hints in some places. You can use mypy to catch a few more bugs proactively.
  • I can see a lot of unnecessary imports. You can use a combination of isort and flake8 to fix these.
  • Also, it would be better integrate black for code formatting.

These linters can be added in as a pre-commit and in GHAs

Here is a PR you can refer to for pre-commit setup - Flagsmith/flagsmith-python-client#70
Also attaching a link for Abstrac Base Classes (ABCs) in Python - https://realpython.com/inheritance-composition-python

tushar5526
tushar5526 suggested changes Dec 6, 2023
View reviewed changes
Copy link

@tushar5526 tushar5526 left a comment

Choose a reason for hiding this comment

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

Interfaces are generally meant to be extended and not make instances of. Maybe you can refactor to making a helper class and define static methods on it.

@KDwevedi
Copy link
Contributor Author

KDwevedi commented Dec 8, 2023 •
edited
Loading

Tasks:

  • Refactor Supabase interface into a helper class
  • setup GHA
  • fixing imports
  • setup pre-commit locally

@KDwevedi
Copy link
Contributor Author

Hey @tushar5526 made some changes

  • set up flake8, black, isort
  • cleaned imports
  • refactored supabase class as supabaseClient

Could you take another look?

@tushar5526
Copy link

Looks fine to me, a bit more nitpicking but I see Supabase().function_call() being used at multiple places. Basically you are trying to namespace all the Supabase helper functions at one place, therefore defining those methods as static methods is much more cleaner.

Also marking the Singleton pattern if you want a global instance of Supabase being used. I am approving the PR - you can do these changes as per the priority of this ticket. Thanks!

@KDwevedi
Copy link
Contributor Author

KDwevedi commented Dec 12, 2023 •
edited
Loading

Appreciate the nitpicking!
I've been trying to look into python patterns so this is good direction.

@KDwevedi KDwevedi merged commit 2ff1fe5 into development Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tushar5526 tushar5526 tushar5526 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KDwevedi @tushar5526