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

Better QA configuration #99

Closed FledgeXu closed 1 year ago

FledgeXu commented 1 year ago

I'm not sure if we still need to stick black and flake8 to the old version and I updated it for now.

This PR also contains some changes to .gitignore, which may not quite fit the principle of one PR for one feature point, but I thought it was a relatively minor change, so I added it.

FledgeXu commented 1 year ago

Can you elaborate on why we'd need that? I think it's harmless but for the sake of harmonization, I'd like to know where it's coming from.

Ok, This is something very annoying that comes with macOS. So When you use the Finder.app open any folder in macOS, this application will automatically create a file in that folder called .DS_Store.

Maybe I can drop this commit and add it to my global gitignore.

rgaudin commented 1 year ago

I know about that one, being on macOS as well. The other ones I understand what they do but is that a list you cooked yourself or is that in the default provided by github or your text editor?

FledgeXu commented 1 year ago

@rgaudin It's form the gitignore.io.

rgaudin commented 1 year ago

@rgaudin It's form the gitignore.io.

Ah that's interesting ; I didn't know this service. What tags did you use? Maybe we could replace it with https://www.toptal.com/developers/gitignore/api/python,macos,windows,linux ? We don't have anything specific in this repo.

rgaudin commented 1 year ago

And please rebase ;)

rgaudin commented 1 year ago

Merging base branch into your feature branch is not rebasing…

FledgeXu commented 1 year ago

Sorry, my fault, i push the branch accidentally😅

FledgeXu commented 1 year ago

Ah that's interesting ; I didn't know this service. What tags did you use? Maybe we could replace it with https://www.toptal.com/developers/gitignore/api/python,macos,windows,linux ? We don't have anything specific in this repo.

Yes, we can. It's a pretty handy website. And I used just the macos tag.

rgaudin commented 1 year ago

Yes, we can.

Please do ; it's a good opportunity.

FledgeXu commented 1 year ago

This PR is ready for review.

rgaudin commented 1 year ago

This PR is ready for review.

Is the re-request review button not working? It's the double arrow one next to reviewer name.

Everything's good but since we're using tox, can you move the isort check into tox.ini so it's consistent ?

The you can rewrite your commit history: we don't need 2 commits for formatting ; squash them together and "Fix a bug in ci." is not an appropriate commit name. Actually it should be squashed as well now.

rgaudin commented 1 year ago

and rebase off main