search

PonyGE / PonyGE2

PonyGE2: grammatical evolution and variants in Python
GNU General Public License v3.0
155 stars 92 forks source link

Pep8 compliance + improved README + LICENSE #121

Closed nbro closed 2 years ago

nbro commented 3 years ago

Here are the changes

  1. I formatted the .py files under src by limiting the number of characters per line to 79 (as explained here). I did this with PyCharm (Code > Reformat Code), after having to set the hard limit to 79). I did not change any other file outside src. Maybe not all lines have been limited to 79 characters (for instance, I didn't touch any string and there are some long strings in some modules: for instance, the regular expressions to parse the BNF grammars have not been touched!). Note that pep8 says that, if you really want, we can have a wider limit, but comments actually should be restricted to 72 characters (which I haven't really checked). Although we may want to have a wider limit (up to 99 chars) for readability, this commit/pull request at least makes the code look more consistent: later, if we want, we can change/increase again the hard limit.
  2. I improved the README file by structuring it more clearly.
  3. I added the full GPLv3 license (as a reference): it's just a copy and paste of what Github generates when you specify the GPLv3 license.
  4. Fixes some typos in comments and renames one variable that contained a typo
  5. Fixes this issue https://github.com/PonyGE/PonyGE2/issues/116; this is a temporary solution, because a better solution may require a significant code re-design/re-factoring, which is something that should be done in a future pull request

This branch started from the other branch here, so, if you merge this pull request, it should also fix the issues that that pull request is supposed to fix. So, if you merge this pull request, you don't have to merge the other.

Note that this pull request does not fix all issues related to pep8-compliance. See this issue for more details. I wanted to incrementally propose improvements, so that I don't introduce big changes.

jmmcd commented 3 years ago

Thanks @nbro! Very good work.

Everyone, let's discuss.

If everyone is ok with this, I will review the changes and aim to accept the PR.

nbro commented 3 years ago

@jmmcd "manely because we like it the way it is." is really a very bad reason to use something. main is widely used in Python. I know it may look funny, but if you want this software package to look serious enough for research, you need to make these changes, including using main instead of mane. I know this may sound like an extreme/exaggerated claim, but there is really no point in using mane when everyone uses main (it's just another source of confusion that can be avoided in the first place).

dvpfagan commented 3 years ago

Its a historical artifact back to the original PonyGE so while it may not be what everyone else does it is what we did and we like to keep it.

Thats just the sentimental bunch we are :D

As for the character limit I detest the 79 character limit in pep8, nobody has managed to make a good reason for it yet, but yeah if we want to be pep8 compliant we need to have it.

Dave

On Wed, 6 Jan 2021 at 15:24, nbro notifications@github.com wrote:

@jmmcd https://github.com/jmmcd "manely because we like it the way it is." is really a very bad reason to use something. main is widely used in Python. I know it may look funny, but if you want this software package to look serious enough for research, you need to make these changes, including using main instead of mane. I know this may sound like an extreme/exaggerated claim, but there is really no point in using mane when everyone uses main.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/PonyGE/PonyGE2/pull/121#issuecomment-755363567, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHTHOR7RI4PVF36GLXYRCTSYR6BFANCNFSM4VOTDGSQ .

nbro commented 3 years ago

@dvpfagan This library does not really try to be backward-compatible, from what I've seen, so there's no point in still using mane (only because it may refer to the mane of a lion or something: I don't really know that reason for this misspelling, but it's not useful at all). Moreover, Python developers really should follow pep8, unless there's a very good reason not to follow it. I also don't fully like the 79 limit, but consistency in a software package (and across software package written in the same language) is very important, so I think we should adopt the 79 character limit, at least for now. In the future, we may decide to change it.

dvpfagan commented 3 years ago

It’s a horses mane and the code is called ponyge, eg a small horse

That’s the reason why

Dave

On Wed 6 Jan 2021 at 15:42, nbro notifications@github.com wrote:

@dvpfagan https://github.com/dvpfagan This library does not really try to be backward-compatible, from what I've seen, so there's no point in still using mane (only because it may refer to the mane of a lion or something: I don't really know that reason for this misspelling, but it's not useful at all). Moreover, Python developers really should follow pep8, unless there's a very good reason not to follow it. I also don't fully like the 79 limit, but consistency in a software package is very important, so I think we should adopt the 79 character limit, at least for now. In the future, we may decide to change it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/PonyGE/PonyGE2/pull/121#issuecomment-755375245, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHTHOQMMWESFG6F7VHDTZ3SYSAGHANCNFSM4VOTDGSQ .