search

goncalopp / simple-ocr-opencv

A simple python OCR engine using opencv
GNU Affero General Public License v3.0
525 stars 175 forks source link

Add License, README changes, .gitignore #17

Closed RedFantom closed 7 years ago

RedFantom commented 7 years ago

The commits in this branch add the GNU AGPLv3 license to the repository, change the formatting of README.md for better readability, two fixes for typos committed by @zuphilip, headers at the top of the files containing copyright information and an updated .gitignore for easier use with the popular IDE PyCharm.

This PR is related to issue #14 . Squash merging this branch with a new commit message is recommended.

goncalopp commented 7 years ago

.gitignore looks good. Ignoring XML might be undesirable in the future, but I see no harm at this point.

LICENCE looks good

I'm not sure about the README - I'll probably line-pick the title and typo correction, but I think the license is already clearly in its own file. We can have a AUTHORS file to keep track of authors - I can add it

Regarding the file headers - they mention Thranta Squadron GSF CombatLog Parser, which is not related to this project. They're mostly redundant, so I'd rather not have them. Also, copyright attribution would be wrong at the point we merge this in

segmentation_filters.py and processor.py contain formatting changes. I'm guessing they would already be included in the pep8 branch? Let's maybe rebase this on top of pep8 after we merge that one in.

RedFantom commented 7 years ago

The Free Software Foundation recommends having a notice like this in the README, so that's why I added that. I can't believe I let that whole GSF CombatLog Parser thing slip through though, I guess I was half asleep at the time or something? Why would copyright attribution be wrong? Does it make the licensing more complicated? The PEP-8 changes should indeed be included in the pep8-branch. It would of course be possible to recreate all changes from scratch in the right order, but it would be quite complicated. This is the amount of splitting that can be achieved by using cherry-pick. To fully conform to PEP-8, the docstrings have to be changed manually when using a tool.

goncalopp commented 7 years ago

I don't understand why it makes a difference from a copyright or licensing perspective, but following the FSF recommendation makes sense. I don't expect most users reading it to care, though - let's keep it at the bottom of the file? Attribution would be wrong because, at the time of the merge of this PR, I think you don't actually own the copyright for any part of the files, excepting the README itself.

Regarding docstrings, I don't think PEP8 recommends having parameter names. Or were you referring to something else?

RedFantom commented 7 years ago

Keeping it at the bottom of the file is OK with me. You're right, I only own copyright for my additions to the code, and those are non-existent in this branch, really. PEP-8 doesn't specifically recommend having the parameter names (inherited from PEP-257), but I think they make a good addition to the docstrings when the short descriptions of the parameters are added after them (I hadn't gotten to that part yet) . The triple double-quotes are recommended by PEP-8.

goncalopp commented 7 years ago

Sorry for the delay getting back to this! I'm getting merge conflicts on this one after we merged the PEP8 PR. Can you try to rebase these on top of current master?

RedFantom commented 7 years ago

@goncalopp If everything went as I think it should have, it should already be rebased, actually. Don't worry about the delay, I'm busy with other things as well. To me it says This branch has no conflicts with the base branch, which I find confusing?

goncalopp commented 7 years ago

To me it says This branch has no conflicts with the base branch, which I find confusing?

Yes, I'm, getting that as well, but when I tried to actually merge the branch it gave me plenty of conflicts. I think I did something wrong, though, because I've retried it today and it worked. It's definitely not rebased, just merged - you can see the commits parallel to master in the network view. It doesn't matter now though :) I'll patch a few things and get back to you

goncalopp commented 7 years ago

Hi, Can you please take a look on the redfantom-readme branch and see if you agree with the changes?

Summary:

If you do, please git cherry-pick 3b23119b65a596446fbd4d72ac5ff8e1fbb06882^..64ac50d2690c19f42096b22a54c12013d410f77f here, and I can merge-squash it.

RedFantom commented 7 years ago

The changes are fine with me, but I think there should be a header in the README file for the project. Are you OK with a header in general, and particularly the header I created in commit f2cd64b?

goncalopp commented 7 years ago

Yes, that looks good. I think this project should be renamed at some point, but that's something for another day :) Thank you for all your work!