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!: complete rewrite #1

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

VincentRPS
Copy link

A complete rewrite of orx.

token: str,
intents: Intents,
state_cls: t.Type[State] = State,
models: dict[Model, Model] = DEFAULT_MODELS,
Copy link
Member

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

Copy link
Member

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?

Copy link
Author

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)
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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,
Copy link
Member

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?

Copy link
Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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:
Copy link
Member

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"}
Copy link
Member

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.

@EmmmaTech
Copy link

Shouldn't this be called a complete write of orx? There's no code to deem this a rewrite.

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.

3 participants