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

Reduce the amount of dependencies #56

Closed
monosans opened this issue Jan 9, 2025 · 10 comments · Fixed by #57, #58 or #60
Closed

Reduce the amount of dependencies #56

monosans opened this issue Jan 9, 2025 · 10 comments · Fixed by #57, #58 or #60
Labels
enhancement New feature or request logging performance

Comments

@monosans
Copy link
Contributor

monosans commented Jan 9, 2025

Only a few simple one-line functions are used from pytelegrambotapi, it can be easily removed. Thanks to this, you can get rid of a lot of dependencies of pytelegrambotapi itself.

docutils and matplotlib don't seem to be used at all (I could be wrong).

Pillow and aiohttp could be made optional by moving them to a separate extras like pip install telegramify-markdown[mermaid].

I also think loguru should be removed, since libraries should use standard logging. The choice of loguru should be up to the end application, not the library. Those who want to use loguru can do so: https://loguru.readthedocs.io/en/stable/overview.html#entirely-compatible-with-standard-logging.

Thanks!
@neutron-nerve neutron-nerve bot added enhancement New feature or request logging performance labels Jan 9, 2025
@monosans monosans changed the title Make unnecessary dependencies optional via extras Reduce the amount of dependencies Jan 9, 2025
@sudoskys
Copy link
Owner

sudoskys commented Jan 9, 2025 •
edited
Loading

[pytelegrambotapi] Due to some open source protocol conflict considerations, I had to introduce it as a third party.

@sudoskys
Copy link
Owner

sudoskys commented Jan 9, 2025 •
edited
Loading

[loguru] #53

Logging function, indeed we don't have a good enough logger. loguru is too bulky

@sudoskys
Copy link
Owner

sudoskys commented Jan 9, 2025

[Pillow] https://github.com/search?q=repo%3Asudoskys%2Ftelegramify-markdown%20Pillow&type=code
Yes, we actually only use the validation function.
I personally don't agree with optional dependencies, I think it may introduce additional complexity.

[aiohttp] For aiohttp, almost all popular robot operation libraries use it

Can you explain which scenarios require strict reduction of dependencies?

@sudoskys
Copy link
Owner

sudoskys commented Jan 9, 2025 •
edited
Loading

Matplotlib and docutils were introduced as latex renderers, but experiments with latex on the client side ended in failure.
*I forgot to remove them :(

@monosans
Copy link
Contributor Author

monosans commented Jan 9, 2025

I personally don't agree with optional dependencies, I think it may introduce additional complexity.

Yes, I agree that it adds complexity, so it's up to you to decide.

@nj-vs-vh nj-vs-vh mentioned this issue Jan 12, 2025
Merged
@sudoskys sudoskys linked a pull request Jan 13, 2025 that will close this issue
Dependency cleanup #57
Merged
@neutron-nerve neutron-nerve

This comment was marked as outdated.

Sign in to view
@monosans
Copy link
Contributor Author

loguru is still not removed, maybe reopen?

@sudoskys sudoskys reopened this Jan 13, 2025
@sudoskys
Copy link
Owner

We should customize a simple logger.
I'm a bit busy though(working on nextjs), so it might take a while...
If you are willing to make a PR, that would be great!

@monosans
Copy link
Contributor Author

If you are willing to make a PR, that would be great!

Will do

sudoskys added a commit that referenced this issue Jan 13, 2025
@sudoskys
🔧 refactor(app): improve logging and exception handling
0ef4e4a 
- Changed `logger.warn` to `logger.warning` for better clarity.
- Added exception handling for `aiohttp` import to manage missing dependencies.

This enhances code readability and robustness.

#56
sudoskys added a commit that referenced this issue Jan 13, 2025
@sudoskys
🧹 refactor(app): update logger import source
d4f6f6b 
Replaced `loguru` logger import with custom logger from `.logger` module. This change improves consistency and maintainability by centralizing logger configuration.

#56
@sudoskys sudoskys linked a pull request Jan 13, 2025 that will close this issue
feat():optional-dependencies, remove pydantic :( #58
Merged
@sudoskys
Copy link
Owner

Now there should be ok

@sudoskys sudoskys linked a pull request Jan 22, 2025 that will close this issue
deps(better): Remove loguru, use logging instead #60
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request logging performance
Projects
None yet
Milestone
No milestone
2 participants
@sudoskys @monosans