search

adasilva / journal2ebook

optimize academic pdfs for e-readers
Other
107 stars 12 forks source link

Complete rewrite to python 3 and general cleanup #35

Closed cgahr closed 2 months ago

cgahr commented 4 months ago

A while ago, I rewrote journal2ebook from python 2 to python 3 since I had problems running it.

In the end, as an exercise, I completely rewrote and cleaned up the codebase, removed duplication and (IMO) generally improved the code.

In now uses ruff and mypy for code linting, pyproject.toml for configuration and installation and generally satisfies the newest python standards. In principle it is also ready for publication on pypi.

I added a .gitignore and removed all build artifacts, they don't really belong in the repo. For this reason, the diff is bigger then it actually needs to be. To make reviewing this commit easier, it might be easier to review the first few commits individually!

This PR is a response to #34.

adasilva commented 3 months ago

Profile saving is not working for me. I can look into it next week.

cgahr commented 3 months ago

Profile saving is not working for me. I can look into it next week.

I just had a similiar issue, upon restarting however, I could not reproduce the issue.

Is it possible that your selection disappeared? Afaik, without a selection, you cannot save the profile. Or should we save all profiles upon clicking save profile?

afonsoguerra commented 2 months ago

Dear @cgahr, just bumped into this and you saved me tons of time! I can confirm that profiles are not working, other than that it is great. Many many thanks for your rewrite, and obviously to @adasilva as well for the original!

cgahr commented 2 months ago

Happy to hear that everything (but profiles) is working for.

I encounter the same bug, but tbh I have no idea how to fix it...

afonsoguerra commented 2 months ago

I have it all working now. I'll try to add it here. It is probably a bit too verbose with the print statements/debug messages that, like breadcrumbs, I left along the way, but it does work :) I also changed the crop lines to red. while not my favourite colour, the default on my system was white which wasn't very readable, feel free to pick another colour for the final release, but make sure you specify something and don't leave the default. All in all, I think this is going to be super useful! I might expand this to allow setting different output resolutions to better match different devices, but not essential for me right now.

cgahr commented 2 months ago

Nice! I think the easiest way to proceed would then be merging this commit first and creating a new PR fixing the profile bug.

What do you think?

afonsoguerra commented 2 months ago

I couldn't quite pull that off (pun intended), but I managed to do a separate PR that integrates all your changes from here... not ideal but I think it should still provide fair attribution and give an opportunity for @adasilva to comment. Have a go and see if I missed anything :) always happy to learn.

cgahr commented 2 months ago

@afonsoguerra I'll address your PR in the pull request.

I added some instructions for people wanting to help develop or fix bugs. In addition, I added a github action to run the pre-commit hooks on automatically on pull request, to enforce a consistent code style.

@adasilva If everything (but the profile bug) looks fine to you, I propose to merge this PR. Afterwards, @afonsoguerra can create a new PR to fix the profile bug. What do you think?

adasilva commented 2 months ago

@afonsoguerra I'll address your PR in the pull request.

I added some instructions for people wanting to help develop or fix bugs. In addition, I added a github action to run the pre-commit hooks on automatically on pull request, to enforce a consistent code style.

@adasilva If everything (but the profile bug) looks fine to you, I propose to merge this PR. Afterwards, @afonsoguerra can create a new PR to fix the profile bug. What do you think?

Agree - @afonsoguerra can create a new PR with the bugfix after merging this one. It will make it easier to review the bug fix.

Thanks, @afonsoguerra for your contribution!

cgahr commented 2 months ago

A couple cleanup comments.

In the future, it would be better to wait to make configuration changes until the original pull request has been merged, it took me longer to get to review of the new stuff than the changes I had already reviewed.

I'm sorry about that. I intended to add some info for future contributers but mixed multiple things together. Do you want me to revert these commits?

Edit: I resolved all your comments.