-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Extra debug logs displaying requests duration #1781
base: master
Are you sure you want to change the base?
Conversation
This is for the Get books feature? |
Yes. This is what I meant when mentioning the "search panel" 😊 |
I am OK with outputting timing for get books fetches in principle, but having scraper/simple.py do it unconditionally based on DEBUG is not acceptable, as this could be used in different contexts in the future. Instead have it done in the get books code itself. |
655d3e5
to
da037e6
Compare
OK, I have edited this PR in order to do so As I have reduced the indentation in many code blocks, |
da037e6
to
2b64808
Compare
The store plugin code is live loaded, your PR would break Get Books for anybody not using up-to-date calibre. Also there seem to be a whole bunch of unrelated changes to logging. Logging how long a search takes for a given plugin should just be a five line patch, in the run method of the SearchThread class in download_thread.py |
Not very encouraging... 😔 |
On Thu, Nov 03, 2022 at 12:50:57AM -0700, Lucas Cimon wrote:
Not very encouraging... 😔
I took the time to reduce a lot of redundant duplicated code in this PR, but that does not sound like a good thing.
You are most welcome to do that, but separately from changing logging.
And it would require some testing to make sure things didnt break.
Also, this is my first contribution to your project, I don't know about this plugin "live loading" mechanism. Where is it documented?
Almost all internet facing code in calibre is live loaded. Recipes,
metadata download sources, get books stores, etc. So when you make
changes to these they should generally be self contained, not depending
on changes to other parts of calibre.
|
b892bf6
to
0bdde99
Compare
24e81d4
to
d13404d
Compare
Hi!
Today I tried to setup Calibre for my mother to use at home,
and it revealed to be very useful (good job!)
but also a bit frustrating on the search panel when the application fetch many websites,
and it takes ages to finish (we have a somehow slow connexion).
Hence quickly setup a dev environment (really easy to set up, good job again!) to try to figure out which online stores are slow to answer and should be disabled.
This resulted in this PR, that adds a bit of extra logging resulting in those lines:
I found it quite useful. Do you think this would make a nice addition to Calibre?
Regards