search

bundestag / gesetze-tools

Scripts to maintain German law git repository
GNU Lesser General Public License v3.0
113 stars 21 forks source link

workflow-test #19

Closed ulfgebhardt closed 3 years ago

ulfgebhardt commented 3 years ago

@darkdragon-001 Wanna take a look at this? I have no clue of python

Related: https://docs.github.com/en/actions/guides/building-and-testing-python

ulfgebhardt commented 3 years ago

image

ulfgebhardt commented 3 years ago

image

jbruechert commented 3 years ago

@ulfgebhardt This looks like Python 2 syntax. I think you need to rebase your branch to get the Python 3 porting commits that are now in master.

jbruechert commented 3 years ago

Apart from the python 2 syntax errors this looks good already. You can probably remove the pytest step for now as we do not have any tests that it could run.

We can add the functionality to run the scripts and extract laws in an additional pull request. It would probably be good to work on one script that runs all the scrapers and converters we have in the correct order first. That would also be easy to test locally. After that works we can simply run that on GitHub actions.

ulfgebhardt commented 3 years ago

image

Could you lint this for me and push to this branch? @JBBgameich

jbruechert commented 3 years ago

Not sure what you mean, fixing some of this warnings would mean a larger refactoring. Do you want me to fix the warnings or to adjust the lint parameters to only show the more important instead of the stylistic warnings?

ulfgebhardt commented 3 years ago

What ever you please - I believe enforcing a code style is wise - but you might disagree. My aim is to get this into master so we have an "objective" reason to merge or not merge certain changes. Furthermore it would allow us to extend the existing tests in the future and have more detailed test going.

jbruechert commented 3 years ago

I agree on the coding style part, but the linter complains about some functions being too complex, and that is not a thing that is very easy to fix

ulfgebhardt commented 3 years ago

As soon as you are satisfied please approve it yourself and merge. That way we have the check in the master and can enforce it before merge

jbruechert commented 3 years ago

The only warning left is "E722 do not use bare 'except'". Exactly for the reason using an except without naming the exception it is bad, I can't fix it right now, as I don't know which exceptions can happen.

ulfgebhardt commented 3 years ago

Thanks alot <3

image

I now enforce the checks, so please rebase