Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code refactor and small bugfixes #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

therealpeterpython
Copy link

I combined #3 and #4 to get the best of both worlds, and updated the code on top of that. The dependencies have changed, so I suspect the nix/poetry files need to be updated, but I have no idea how to do that. Maybe you could help @mmarx?

We could also add a license to the project to make the permitted type of use clearer, but I'm not sure which would be best.

- added more docstrings and comments
- use more pathlib functions
- replaced the pandas dependency with csv from the standard library
- made edge storage part of class functionality
- added constants to control header, timeout and user interaction
- rewrote parts of the code to be more pythonic and robust
- improved error handling
#else:
# return []
# TODO: Don't hard-code URLs
# TODO: What is the seenotrettung about?
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at the time we created this project, there were some issues with etherpad servers. If a pad broke, one could still obtain the content via some "hack" (history mode or similar) and copy-paste it into a new one (seenotrettung called that time, term was created/evolved during a plenum) which would be the go-to pad then.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Can I remove that part from the code then?

@nicoa
Copy link
Owner

nicoa commented Jan 3, 2024

Looks really good (besides the mentioned points in your above comment), thanks a lot for the work here!

I think regarding dependencies that should be fine for now, as you did not add any new depedneices which aren't in the standard library.

Regarding license: good point, will think about adding one.

@mmarx
Copy link
Contributor

mmarx commented Jan 3, 2024

The dependencies have changed, so I suspect the nix/poetry files need to be updated, but I have no idea how to do that. Maybe you could help @mmarx?

nix takes the dependencies straight from poetry, so it suffices to update those (strictly speaking, this is not even required as you just removed some external dependencies, but this will speed up building and shrink the nix closure): simply remove pandas and pathlib2 from pyproject.toml and run poetry update, then make sure you commit the updated poetry.lock.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants