mansuf / mangadex-downloader

A command-line tool to download manga from MangaDex, written in Python.
https://mangadex-dl.mansuf.link/
MIT License
472 stars 37 forks source link

Add stacked progress bar and ability to change logging level #72

Closed mansuf closed 1 year ago

mansuf commented 1 year ago

Summary

Close #65

mansuf commented 1 year ago

Thanks for the review @picnixz

I will try to fix the codes from your review :)

To be honest, i'm not great at refactoring the code. I usually working alone when coding. So as long as it's working i'm fine with it. I don't know how to make code looks readable to other developers, even though i tried (when developing version 2.0). It's still not enough and the codes is barely unreadable.

So when security vulnerability is found in version 1.3.x when i'm trying to refactor for v2.0. I learned something that good code affects security.

picnixz commented 1 year ago

I don't know how to make code looks readable to other developers

Use linters like pylint / ruff. They will probably tell you that there are many things to change, but you can change them step by step, or see what would be "good practices".

If you want to read some literature on the topic, there is Clean Code by Robert C Martin. Although it is intended for Java, you can find some ideas out there. A good practice is actually to dive into the Python library itself and see how things are done in general. Or in frameworks such as Django or SQLAlchemy.

mansuf commented 1 year ago

I've fixed some typo in the codes from your review. As for option to change logging level, i think i will stick with --log-level instead of -q and stacked -v options. It's more convenient in my opinion to use --log-level rather than -v and -q. Also, the --log-level value can be stored in config.

picnixz commented 1 year ago

It's more convenient in my opinion to use --log-level

In general, for CLI programs, it is very natural to repeat -v like -vvv to raise the verbosity. You can store the verbosity count in the logging configuration as an integer, e.g., verbosity: 1 is equivalent to -v.

The -v and -q flags are common flags in general (most of the Unix CLI programs use this convention). Similarly, the -V flag is a common alias for --version.

If you want to stick with a --log-level, then it is also common to support environment variables as well (e.g., LOG_LEVEL=INFO python prog.py) but this is not an absolute feature.

Anyway, since you are the owner, it's up to you in the end !

mansuf commented 1 year ago

I've made up mind that i will stick for --log-level option. Also i fixed some grammar errors and bugs.

This PR is ready to test. I will merge this PR once the test results are good.

Ignore the merge conflicts, i will fix that later

picnixz commented 1 year ago

Thank you. I tried before the last commit and it looked good to me. I don't know what the default behaviour is but I think you can ensure that users don't see changes unless they enable this new feature.

Unfortunately, I will be away until mid July so feel free to merge. If there are remaining things to do, we could do it in July.

mansuf commented 1 year ago

I don't know what the default behaviour is but I think you can ensure that users don't see changes unless they enable this new feature.

I made deprecation warning when users are using deprecated option, so the deprecated options is still working until specified version are reached.