search

openzim / python-scraperlib

Collection of Python code to re-use across Python-based scrapers
GNU General Public License v3.0
19 stars 16 forks source link

Issue 155: log metadata prior to verification #160

Closed richterdavid closed 3 months ago

richterdavid commented 5 months ago

Fix #155

There's a few things not working right yet, and this isn't passing pre-commits yet, so I'm pushing this just for advice at this time.

richterdavid commented 4 months ago

Now passing tests and precommits.

rgaudin commented 4 months ago

@richterdavid I see you've resolved all conversations. Is it ready for re-review?

Also, our convention is that the PR submitter only resolves a conversation should it have been fixed in code directly. If the conversation was a question or if it leads to an explanation or a different way to address, then it's to the reviewer (who started the conversion) to resolve.

richterdavid commented 3 months ago

Yes, please do re-review.

I think my resolves complied with that convention. Feel free to reopen any you see fit.

benoit74 commented 3 months ago

@richterdavid we would like to release 3.4.0 in the coming days, we would appreciate if you could achieve fix the few remaining conversations since 3.4.0 ; otherwise this PR will be postponed to 3.5, which is not a concern.

richterdavid commented 3 months ago

Please take another look.

benoit74 commented 3 months ago

@rgaudin could you maybe propose a commit with what seems reasonable to fix your points. It looks like we are stuck in a silly conversation on "best design" which I've never seen leading to anywhere but lengthy debates. Since we are the maintainers of this lib on the long term (we will have to "pay" for improper design) and since you have more experience on this project, I think that you can deserve to have the "last word".

rgaudin commented 3 months ago

Will do

rgaudin commented 3 months ago

See #172 as I can't push here

benoit74 commented 3 months ago

Superseeded by https://github.com/openzim/python-scraperlib/pull/172