search

bundestag / gesetze-tools

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

Changes of joacmue #14

Open ulfgebhardt opened 3 years ago

ulfgebhardt commented 3 years ago

Hey @joacmue

would you be interested in getting your changes in the master? Would that be applicable?

Interested in becoming a maintainer?

joacmue commented 3 years ago

Hi @ulfgebhardt, sure, merging that back into your fork would totally work for me. It just seemed pretty dead over the past months ;) I'm not that versed around open source development, so... should I start a PR from my fork to yours?

Not sure what I'm getting myself into by becoming a maintainer, but I'd be open to contribute.

joacmue commented 3 years ago

Looking over the other current activities, this one mainly fixes lawdown and adds some image/table features to it (my test case was the StVO which is pretty heavy on images in tables for Traffic Signs and stuff). Might only want to review the changes to lawdown and readme.md and just throw away the other changes. I tried going into the banz_scraper but ran into issues with their page redesign pretty fast. If I recall correctly, I mainly noted down some new candidate links, but stuff seems to be better addressed by #18

ulfgebhardt commented 3 years ago

Hey @joacmue nice to hear from you. I believe I could incite some activity into the project lately. I was informed that you made some nice changes not to long ago and just wanted to expose those to this repo.

Would you be interested i contributing? I believe we are on a good way to get things running and have an up to date https://github.com/bundestag/gesetze repo soon. With your help sooner and better <3

With the looming Election this year having the repo up to date would save me from millions of questions why its not up to date ;)

joacmue commented 3 years ago

So, I just rebased my fork to what happened here but did not get the whole pathlib stuff right away, so most of the changes made there might have gotten lost. Feel free to add them back during the review.

joacmue commented 3 years ago

So I just recognized the bigger picture of pushing the actual laws to https://github.com/bundestag/gesetze That puts the work here in a new perspective for me. please stand by while I process this. Kidding aside, once my slightly more involved state stuff in the parser got merged back, I'll see where I feel comfortable helping out on issues. Might need some help with the actual scrapers, though.

ulfgebhardt commented 3 years ago

I believe we have to copy the PR into the repo or rebase in oder for the tests to run. I cannot accept your suggestions. I think only @joacmue can, since hes the owner of the fork.

darkdragon-001 commented 3 years ago

@ulfgebhardt We should give @joacmue some time to work on this...

darkdragon-001 commented 3 years ago

@joacmue Please check which changes are on top of #24 which restored BAnz functionality and adapt your PR accordingly.

ulfgebhardt commented 3 years ago

I am not sure we can wait @darkdragon-001 since we have so many changes pushed into master. Every commit we make will get us further away from merging this. It is fine with me to just leave this open and cherry pick commits or changes and PR them individual, but then we will not honor the work of @joacmue . If you want to take the effort and see the benefit to get these changes in, please just take over. -> Tho thats just my opinion ;)

joacmue commented 3 years ago

Sorry, quite busy at the office right now. I hope to look into rebasing from the original repo to my fork by the weekend. Should be possible to merge after that.

ulfgebhardt commented 3 years ago

We would love to have you involved @joacmue ! I believe the others have the sam struggle and have to work during the week. I hope that we get another of those sessions like last weekend for the coming one and have the thing in a state where it can do stuff automatically without much human interaction. <3

joacmue commented 3 years ago

Yeah, we're in some sort of Deadlock right now. I don't have write access to merge anything here but just invited you as collaborators to my fork. I'll try and merge locally with the current master here to get rid of some of the conflicts currently present.

I honestly don't know how to run the tests, though. Can you point me to some howto? It might actually be worthwhile to make a branch here and bend this PR onto it. I guess we could then run the tests defined here and have less issues with differing writing access...

ulfgebhardt commented 3 years ago

I think it would be wise to have a rebase on your branch @joacmue . I tried, but its quite complicated and out of my league, since it needs knowledge of what you have done, which parts are important which are not.

joacmue commented 3 years ago

Thanks for the hint of rebasing, @ulfgebhardt, I wasn't aware that this is available across forks. As I said, pretty new to this stuff. Please excuse the inconvenience. I did manage to rebase everything, while silently cursing the back-then-Joachim but now everything seems to be up and running again. I still don't quite know how I could start the checks, but I guess that I'm lacking the rights to perform those (given they are defined on the "original" gesetze-tools but need to run over my changed scripts.

joacmue commented 3 years ago

So, all conflicts are resolved, the code is rebased to the state of the master and the checks are all passing on my fork. I don't quite know how to get the checks running here, but I guess actually merging this is out of my hands?

darkdragon-001 commented 3 years ago

@ulfgebhardt @JBBgameich Can someone have a look why the checks aren't running for merge requests from fork? Can I trigger those manually?

darkdragon-001 commented 3 years ago

@joacmue I added some small changes to your branch. Feel free to have a look and leave a comment if you see things differently. I will have a closer look at lawdown.py the next days.

Note to myself: When merging this, we should use the squash merge function to maintain a clean git history and squash this many commits in a single patch.

ulfgebhardt commented 3 years ago

I believe checks are not running because of security reasons. As soon as we agree to merge this, I will just push a branch with your changes and @darkdragon-001 can approve it, we merge it. As soon as you have at least 1 commit in the Orga I will give you member access, which will allow you to push to this repo. So forking will not be needed in the future.

Just tell me when your done & ready. That includes @darkdragon-001 : Would you approve?

joacmue commented 3 years ago

@joacmue I added some small changes to your branch. Feel free to have a look and leave a comment if you see things differently. I will have a closer look at lawdown.py the next days.

Note to myself: When merging this, we should use the squash merge function to maintain a clean git history and squash this many commits in a single patch.

Changes made work for me. Also, squashing will definitely make sanse here :D

joacmue commented 3 years ago

As soon as you have at least 1 commit in the Orga I will give you member access, which will allow you to push to this repo. So forking will not be needed in the future.

@ulfgebhardt What do you mean here? Should I be doing something?

darkdragon-001 commented 3 years ago

Just tell me when your done & ready. That includes @darkdragon-001 : Would you approve?

I will finish the review over the weekend and approve when everything looks fine.

ulfgebhardt commented 3 years ago

Check ran now since I pushed stuff to a branch in this repo. Please just update the branch and then merge this PR if everything is cool @darkdragon-001 .

See: https://github.com/bundestag/gesetze-tools/pull/38

ulfgebhardt commented 3 years ago

Depending on how interested @joacmue is to get things done, cherry picking might be a good option here @joacmue I also made a copy of this branch to here: https://github.com/bundestag/gesetze-tools/pull/38 That way the tests do run and it is mergable

darkdragon-001 commented 3 years ago

@ulfgebhardt I updated the CI to also run on PRs (see #44). When it is merged, I will pull in master to activate those changes. You can close #38 then.

joacmue commented 3 years ago

Depending on how interested @joacmue is to get things done, cherry picking might be a good option here @joacmue I also made a copy of this branch to here: #38 That way the tests do run and it is mergable

We could try that, but I've never done that and fear that my development on the fork was too erratic and chaotic to do some reasnable cherrypicking on the basis of commits. I just lacked the discipline of doing only one thingin each commit. I guess I'll have to figure the remaining issues out to merge everything.

darkdragon-001 commented 3 years ago

Depending on how interested @joacmue is to get things done, cherry picking might be a good option here @joacmue

We could try that, but I've never done that and fear that my development on the fork was too erratic and chaotic to do some reasonable cherrypicking on the basis of commits. I just lacked the discipline of doing only one thingin each commit. I guess I'll have to figure the remaining issues out to merge everything.

I totally agree. I think it is lots easier to just fix the few remaining issues instead of looking over those >50 commits. Most things are working well, just some minor glitches on the way :wink:

ulfgebhardt commented 3 years ago

You guys are very constructive and do really great work ❤️ @joacmue If interested join our discord channel: https://discord.gg/YcQSvKcKB2

joacmue commented 3 years ago

After some more meddling around, all comments should now be adressed. Thanks for the hints on where to clean up my stuff.

ulfgebhardt commented 3 years ago

@darkdragon-001 what you say? merge?

joacmue commented 3 years ago

So, I rebased to the master from here. lawdown.py still works on everything wth the expected results, so I guess I made that right.

However, I noticed that the banz_scraper fails with an error. I copied over the code from master here and I do have the same error, so I guess there's some fault in the code there. Not gonna put cleaning that up in this PR, though ;)

darkdragon-001 commented 3 years ago

Tables look good now :+1: Thanks for your great work so far!

I resolved all comments which are good now and commented on the remaining ones. Especially the regression mentioned in https://github.com/bundestag/gesetze-tools/pull/14#discussion_r632982072 should be resolved before merging IMHO.

ulfgebhardt commented 3 years ago

Can't we make Issues for the remainign Issues and merge? This PR is way to big already

4350pChris commented 2 years ago

Any update on this?