-
Notifications
You must be signed in to change notification settings - Fork 1
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!: complete rewrite #1
base: master
Are you sure you want to change the base?
Conversation
token: str, | ||
intents: Intents, | ||
state_cls: t.Type[State] = State, | ||
models: dict[Model, Model] = DEFAULT_MODELS, |
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.
Defaults are mutable. Check for None and set to the default instead
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.
Also what is this structure? Model to Model?
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.
Also what is this structure? Model to Model?
Guild: MyGuild
@@ -1,30 +1,20 @@ | |||
# The MIT License (MIT) |
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.
I assume template will eventually be restored?
|
||
from typing import TYPE_CHECKING | ||
from typing import Literal |
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.
I prefer locking all typing imports inside a if TYPE_CHECKING
block
|
||
|
||
import asyncio | ||
import typing as t |
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.
Please follow how its done in nextcore
self, | ||
token: str, | ||
intents: Intents, | ||
state_cls: t.Type[State] = State, |
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.
Do we need a state class? Cant it be in Bot?
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.
I like to separate State from Bot to keep the Bot interface clean for users
@@ -0,0 +1,64 @@ | |||
# cython: language_level=3 |
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.
I generally prefer the java approach of one thing per file.
@@ -1,2 +0,0 @@ | |||
# Marker file for PEP 561. | |||
# This marks that we use inline typings for type checkers |
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.
Needed for mypy support iirc
@@ -0,0 +1,113 @@ | |||
# cython: language_level=3 |
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.
I dislike the idea of a default cache.
I want to encourage users to cache the least amount possible. Default-caching everything is something I dont want. At all.
A caching impl that only stores classes a user explicitly opts in to is fine, however I would prefer the user implements their own caching.
import typing as t | ||
|
||
|
||
class _stored: |
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.
I hate this class name
types-orjson = {version = "^3.6.2", optional = true} | ||
discord-typings = "^0.5.0" | ||
python = "^3.11" | ||
nextcore = {git = "https://github.com/nextsnake/nextcore"} |
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.
Nextcore is out on pypi! Its fine to use nextcore from it.
Shouldn't this be called a complete write of orx? There's no code to deem this a rewrite. |
A complete rewrite of orx.