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

Big fixes and improvements #22

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

Big fixes and improvements #22

wants to merge 15 commits into from

Conversation

yspreen
Copy link

@yspreen yspreen commented Nov 14, 2019

Fixes line endings, add crc, add automatic folder search, add link to nginx with the mod.

@yspreen
Copy link
Author

yspreen commented Nov 14, 2019

Made the commits pretty small, you can basically see a list of changes in the messages.

@travcunn
Copy link
Owner

Hey, thanks for submitting this change! I'll see if I can put together some test cases. I haven't maintained this package for a while but I'd like to improve it if you are using it.

@yspreen
Copy link
Author

yspreen commented Nov 14, 2019

Yeah, so I tried to just make it easier to use the package at first, adding the convenience class for folders. But then I quickly found out that something didn't work.

I tried adding CRC at first, but the issue persisted. Turns out the zip module may have changed (?) and now requires \r\n.
After finding that out and fixing it, I decided to create the PR.

For the line endings, I found out through the tip at nginx:

Each line should have no whitespace before the CRC-32 field. At the end of the line, rn. No empty or extra lines.

@yspreen
Copy link
Author

yspreen commented Nov 14, 2019

Btw, quick research into pathlib yielded, that the functionality was added in Python 3.4.
It probably works in Python 2 too, but I didn't test it. Since 2 is ending its support in less than 2 months I think it should be fine to remove the compatibility.

@yspreen
Copy link
Author

yspreen commented Jun 15, 2020

@travcunn I still have success using my own fork of this. Merging might still be helpful to others though :)

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.

2 participants