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

🏗️ Using normalized package architecture #21

Merged
merged 19 commits into from
Jul 9, 2022

Conversation

Sigmanificient
Copy link
Collaborator

This pr changes the current code base to follow a standard python package architecture.

Main changes

  • Added setup config for local installation
  • Moved config & data files to a dedicated vars directory
  • Improved python code and structure
  • Moved run to a package entry point
  • Updated documentation
  • Added a Makefile for easier GNU/Linux setup

@Sigmanificient Sigmanificient requested a review from drawbu July 9, 2022 04:22
@drawbu
Copy link
Owner

drawbu commented Jul 9, 2022

Why deleting all the -> None ?

@drawbu
Copy link
Owner

drawbu commented Jul 9, 2022

Why is there an empty file drawbot/cogs/__init__.py


```json5
Copy link
Owner

Choose a reason for hiding this comment

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

Why deleting this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I put a config.json.example file people can directly copy and rename. Since the content is within the example file, it isn't much useful in the readme, i believe

Plus having a example file within the vars folder should avoid people to try creating a config.json at another locater, i hope

Copy link
Owner

Choose a reason for hiding this comment

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

Yep it will be easier for some, but harder for a majority. People using this bot are in majority beginners, so we need to make this accessible. I'm just gonna adding it back, and we'll have both available

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure

@@ -1,9 +1,5 @@
from typing import Dict, Any, List, Union
Copy link
Owner

Choose a reason for hiding this comment

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

Just put this into the utils/ folder

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sound fair enough, could move it when i'll be at home

@Sigmanificient
Copy link
Collaborator Author

Why is there an empty file drawbot/cogs/__init__.py

This insure cogs to be a sub-package part of the whole drawbot package and thus being included in the installation process

@drawbu
Copy link
Owner

drawbu commented Jul 9, 2022

Yeah, but why leave it empty?

@Sigmanificient
Copy link
Collaborator Author

Sigmanificient commented Jul 9, 2022

Yeah, but why leave it empty?

There is nothing to really put in that init file since discord will use dynamic import for cogs.

I believe that is more a design problem from the lib, as it would has been smarter to handle object, like the nextcord fork did, cause less "python weirdness" and a overall better code structure & logic.

@Sigmanificient Sigmanificient requested a review from drawbu July 9, 2022 15:15
@Sigmanificient Sigmanificient added the enhancement New feature or request label Jul 9, 2022
@drawbu drawbu merged commit dc08861 into drawbu:main Jul 9, 2022
@Sigmanificient Sigmanificient deleted the pkg branch July 10, 2022 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants