Skip to content
This repository has been archived by the owner on Feb 29, 2024. It is now read-only.

Switch to pep-8 #717

Closed
Cerno-b opened this issue Mar 7, 2021 · 11 comments
Closed

Switch to pep-8 #717

Cerno-b opened this issue Mar 7, 2021 · 11 comments

Comments

@Cerno-b
Copy link
Contributor

Cerno-b commented Mar 7, 2021

The codebase seems to adhere to the camelcase notation generally used in C++, likely to keep in line with Qt's function naming.

I was wondering whether you might be open to switching to the Python-recommended naming scheme defined in the PEP-8 https://www.python.org/dev/peps/pep-0008/

Of course, PEP-8 clearly states in the beginning that it should not be applied blindly, however this has become a somewhat standard in the python world that most code analysis tools support and it makes for a more unified and well-readable code base.

I would offer to make the conversion to PEP-8 for labelImg if you would go along with it, but I understand that this is a huge change that touches almost all code files and will cause pending pull-requests to be affected, so it might not be high (or at all) on your priority list.

I would probably do this in multiple steps, with the first step being to convert all the variable and function names to snake-case, so that each line of code stays exactly where it is, which would make adapting outstanding pull-requests easy without breaking anything. Later, I would make some style changes that actually improve code readability so that all warnings that a tool like lint would report, vanish. This makes future bugs easier to identify and avoid while coding.

Please let me know if you are open to the idea of switching to PEP-8.

@tzutalin
Copy link
Contributor

tzutalin commented Mar 7, 2021

Sounds good to me.

Maybe you can leverage a tool to do this conversion

@Cerno-b
Copy link
Contributor Author

Cerno-b commented Mar 7, 2021

@tzutalin Pretty sure there is a way to do this automatically, otherwise PyCharm has a semi-automatic mode that works fairly well. Should be a few hours of manual work.

Do you have any preference when you would want to do this? Are there any outstanding pull requests that you want to merge beforehand? Merging anything afterwards that has been branched off a pre-pep8-state could be a bit of a pain, since almost every line of code will be touched in the conversion.

@Cerno-b
Copy link
Contributor Author

Cerno-b commented Mar 8, 2021

@tzutalin Okay, I'm done with the first step. Do you have some sort of checklist for testing whether everything works? I've run the unit tests but it seems like two failed even before my change. I could play around with the tool for a bit to see if everything seems to work fine, but if you have a more robust way of testing, I'd like to double-check

@tzutalin
Copy link
Contributor

tzutalin commented Mar 8, 2021

We don’t have auto integration tests yet. You need to do manual integrations at the moment.

Basically, you can do these manual tests:

  1. Open the app, load images and label images, and save them. You should be able to save annotation files. Make sure that you can save them for XML , yolo format, and so on.
  2. When you label, make sure you can resize rect and rename them
  3. Make sure hotkeys shown in README can work
  4. Try to check all features in the menu can work. For example, ‘auto save’ can work

@Cerno-b
Copy link
Contributor Author

Cerno-b commented Mar 9, 2021

@tzutalin It looks fine, except for the the CreateML format which seems to have problems saving and loading. I cross-checked with the master branch before my changes and the bug is there as well. I will open a separate issue for this

I did all the checks you suggested and my changes work without problems, so I'll open a PR for you to review

@Cerno-b Cerno-b mentioned this issue Mar 9, 2021
@Cerno-b
Copy link
Contributor Author

Cerno-b commented Mar 10, 2021

@tzutalin Currently working on stage two by removing more warnings and making the code a bit prettier.
Do you still need the dependency to Qt4 and Python 2? I could just refactor them out while I am at it.

I found a few loose ends that could point towards undesired behaviour, I'll mark them as to discuss when I open the second PR after the first one passes.

@tzutalin
Copy link
Contributor

Hi Cerno,
Thanks for doing this. I would rather keep python2 support at the moment. We can refactor step by step. Let’s refactor coding and naming convention first.

@Cerno-b
Copy link
Contributor Author

Cerno-b commented Mar 14, 2021

@tzutalin Sure, no problem. I'm pretty much done with the changes to stage 2 (need to do some thorough testing though). I have it on a separate branch and will create a PR once the current PR is merged.

Not trying to pressure you or anything, but could you give me some insight on how much time it generally takes you to review and merge a PR?

@tzutalin
Copy link
Contributor

Hi
I just saw your pull request. I will check it today

#720

@tzutalin
Copy link
Contributor

PR looks good to me.
Thanks for great work again!

@Cerno-b
Copy link
Contributor Author

Cerno-b commented Mar 22, 2021

I added a separate issue for stage 2: #725

It has its own Pull Request: #723

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

No branches or pull requests

2 participants