Restart the library #209
Restart the library #209
Conversation
|
Since more changes are incoming making this a draft should be a good idea |
|
If you're going for a rewrite you must define strong rules from the beginning to follow. It would be nice to set a linter and ensure mypy compliance for example. Before it becames too late |
I was thinking of using PyRight instead, MyPy is confusing with its errors. It defaults to |
Well you should because mypy is behind of pyright in terms of support with newer typing features. |
|
|
EpikCord/client/buckets.py
Outdated
| ... | ||
|
|
||
| def clear(self): | ||
| """Pretends to clear the Event""" |
There was a problem hiding this comment.
Took a look at PEP257 and I need to do some major changes to docstrings, majorly making them commands.
There was a problem hiding this comment.
I'm not sure on how much you should follow it, though, making docstring "a phrase ending in a period." would definitively improve them, in their consistency and clearness.
EpikCord/client/buckets.py
Outdated
| @@ -10,18 +10,26 @@ | |||
|
|
|||
|
|
|||
| class MockBucket: | |||
| """A mock bucket that does nothing.""" | |||
There was a problem hiding this comment.
Make it an abstract class or a protocol
There was a problem hiding this comment.
Alr, I'll do that soon. I was thinking about it.
EpikCord/client/buckets.py
Outdated
| async def wait(self): | ||
| """Pretends to wait for the Event to be set""" | ||
| ... | ||
|
|
||
| def clear(self): |
EpikCord/client/client.py
Outdated
| @@ -3,7 +3,27 @@ | |||
|
|
|||
|
|
|||
| class Client: | |||
| """The main class of EpikCord. Use this to interact with the Discord API and Gateway.""" | |||
| def __init__(self, token: str, intents: Intents, *, version: int = 10): | |||
There was a problem hiding this comment.
I dont like magic number, it should be extracted to a final int constant.
There was a problem hiding this comment.
Yeah I guess that makes sense
| token: str | ||
| The token of the bot. | ||
| intents: Intents | ||
| The intents of the bot. |
There was a problem hiding this comment.
This is way too generic, no one want to read that
There was a problem hiding this comment.
Yeah I can be more specific with it. Although being that vague also isn't too bad, it's the token of your bot.
There was a problem hiding this comment.
This is a great place to remind people on how to retrieve the token, (maybe as an url to a side page), the importance of not sharing it etc. Boring text, but still preventive for the few that are sage enough to take a read through it.
There was a problem hiding this comment.
Documentation is mean to provide the developer the core knowledge about a library in its abstract form. It is also a way to share advice on code management and good practice, optimization trick for the more pickiest.
Unfortunately, we have too many of those automatically generated docs the give you blind sentence that bring next to no information and just describe what you already know. This is the role endorsed by the argument names, not the role of a documentation. And this is why peoples are taking less and less attention to them because reading two or three time the same piece of into is exhausting.
EpikCord/client/http.py
Outdated
| method: str | ||
| The method of the route. |
There was a problem hiding this comment.
Method could be a Enum type
There was a problem hiding this comment.
I do not want to have to do Route(Methods.GET, URL) for everything...
There was a problem hiding this comment.
I do not want to have to do
Route(Methods.GET, URL)for everything...
And this is why partials exists
There was a problem hiding this comment.
from functools import partial
GetRoute = partial(Route, method=Methods.GET)See the partial documentation.
EpikCord/client/http.py
Outdated
| url = response.url | ||
| method = response.method | ||
| bucket_key: str = f"{method}:{url}" | ||
|
|
||
| bucket: Optional[Union[Bucket, TopLevelBucket]] = None |
There was a problem hiding this comment.
Avoid using types that are too too much complex
There was a problem hiding this comment.
How is that complex? It does what it says...
There was a problem hiding this comment.
By having a typehint of Optional[Union[Bucket, TopLevelBucket]] you have to deal why 2 different interfaces at the same time while being sure that you are not in a None case. This makes more guards statements that could have being avoided by having a better separation of variables, a protocol that define a middle ground interface of the two objects, or other techniques. Reducing the type will always makes the program simpler and easier to read. You dont have to clutter you mind with the correctness of the types and can focus on the code execution.
EpikCord/client/http.py
Outdated
| channel_id=major_parameters.channel_id, | ||
| guild_id=major_parameters.guild_id, | ||
| webhook_id=major_parameters.webhook_id, | ||
| webhook_token=major_parameters.webhook_token |
There was a problem hiding this comment.
Just Make TopLevelBucket Inherit from MajorParameters
There was a problem hiding this comment.
That makes no sense though; MajorParameters is a class that groups all of the major parameters into a single class, a TopLevelBucket is a bucket used for handling rate limits.
There was a problem hiding this comment.
And the TopLevelBucket uses all the attributes of the MajorParameters in order to work.
Inheritance a mechanism where you can to derive a class from another class for a hierarchy of classes that share a set of attributes and methods
There was a problem hiding this comment.
I don't like the hierarchy tree for that though.
MajorParameters
|
|
|
TopLevelBucket
|
Can't we just make a rewrite branch and add it to the milestone thing (Projects)? |
What a nice idea |
Description
The library is very messy and outdated. It needs a rewrite. It has been a few months since I have actively worked on the library, and so I don't know where exactly I am now. Restarting the library will solve all these issues.
Checklist