barefoot
barefoot copied to clipboard
bmwcarit
Python map import scripts refactor
Hi guys,
I'm working on figuring out how Barefoot really works underneath. While I was looking through the Python scripts that import map data, I figured they could use some work. I wanted to show my work in progress to maybe get some comments if and how this could be merged in main stream Barefoot when finished.
What I'm working on:
- Mainly making it DRY
- Making it Python 3 compatible
- Improve readability
- Keep functionality the same
I know this will be hard to review because the changes are so large. But know that I'm not changing functionality in this PR.
I touched all the Python code and finished a first refactoring. It can be better, but it is functional now.
Thanks for refactoring and the PR!
@jongiddy Would you mind having a look on the changes? You're much better in Python than me. I have looked through it, e.g., if conditions, test execution, etc., and it seems all okay (not bypassing any test) and the CI system checked it with perfect results, i.e., also the result of the sample data looks the same as before.
@Conengmo Have you used some code style configuration? If so, could you include that in the repo in order to have the same style with future edits? Further, is the PR still WIP?
Good to hear you want to consider this PR. I follow regular PEP8, the default style guide for Python:
https://www.python.org/dev/peps/pep-0008/
Is there a line width you want to stick to in this project? I tried to keep most at 80 characters, but allowed for lines to go to 100 characters. Personally I like 100.
If you want to consider merging this, I'll take this weekend to polish it a bit more before dropping the WIP. Let me know if you have any comments or requests!
In the Java code style, we use a line width of 100 characters. PEP8 is perfect and can be integrated into the Travis CI configuration. Thanks!
Does Travis also do code style reviews? Another project I work on uses Travis for testing and Stickler for automated code style review:
https://stickler-ci.com/
Alright I finished cleaning up. The Travis test passed, so that's nice. I hope you guys like it, let me know if you have any further requests or suggestions.
Glad you like it. I agree on adding more tests, and also a good idea to add a pep8 check to Travis. But that's something for a new PR I reckon. I'll leave this PR as is so it won't have to be reviewed again.
