gesetze-tools icon indicating copy to clipboard operation
gesetze-tools copied to clipboard

Changes of joacmue

Open ulfgebhardt opened this issue 3 years ago • 33 comments

Hey @joacmue

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

Interested in becoming a maintainer?

ulfgebhardt avatar Mar 26 '21 17:03 ulfgebhardt

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 avatar Mar 28 '21 09:03 joacmue

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

joacmue avatar Mar 28 '21 10:03 joacmue

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 ;)

ulfgebhardt avatar Mar 28 '21 15:03 ulfgebhardt

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 avatar Mar 28 '21 16:03 joacmue

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.

joacmue avatar Mar 28 '21 16:03 joacmue

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.

ulfgebhardt avatar Mar 29 '21 21:03 ulfgebhardt

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

darkdragon-001 avatar Mar 29 '21 23:03 darkdragon-001

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

darkdragon-001 avatar Mar 29 '21 23:03 darkdragon-001

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 ;)

ulfgebhardt avatar Mar 30 '21 00:03 ulfgebhardt

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.

joacmue avatar Mar 31 '21 06:03 joacmue

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

ulfgebhardt avatar Mar 31 '21 11:03 ulfgebhardt

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...

joacmue avatar Apr 03 '21 16:04 joacmue

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.

ulfgebhardt avatar Apr 03 '21 18:04 ulfgebhardt

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 avatar Apr 06 '21 11:04 joacmue

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?

joacmue avatar Apr 06 '21 14:04 joacmue

@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 avatar Apr 06 '21 21:04 darkdragon-001

@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.

darkdragon-001 avatar Apr 06 '21 21:04 darkdragon-001

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?

ulfgebhardt avatar Apr 07 '21 05:04 ulfgebhardt

@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 avatar Apr 09 '21 11:04 joacmue

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?

joacmue avatar Apr 09 '21 11:04 joacmue

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.

darkdragon-001 avatar Apr 09 '21 11:04 darkdragon-001

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 avatar Apr 09 '21 12:04 ulfgebhardt

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

ulfgebhardt avatar Apr 18 '21 10:04 ulfgebhardt

@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.

darkdragon-001 avatar Apr 18 '21 10:04 darkdragon-001

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.

joacmue avatar Apr 18 '21 16:04 joacmue

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:

darkdragon-001 avatar Apr 19 '21 07:04 darkdragon-001

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

ulfgebhardt avatar Apr 19 '21 09:04 ulfgebhardt

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

joacmue avatar Apr 25 '21 15:04 joacmue

@darkdragon-001 what you say? merge?

ulfgebhardt avatar Apr 27 '21 09:04 ulfgebhardt

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 ;)

joacmue avatar May 15 '21 14:05 joacmue