search

bmwcarit / barefoot

Java map matching library for integrating the map into software and services with state-of-the-art online and offline map matching that can be used stand-alone and in the cloud.
Apache License 2.0
665 stars 186 forks source link

Python map import scripts refactor #109

Closed Conengmo closed 2 months ago

Conengmo commented 6 years ago

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:

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.

Conengmo commented 6 years ago

I touched all the Python code and finished a first refactoring. It can be better, but it is functional now.

smattheis commented 6 years ago

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?

Conengmo commented 6 years ago

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!

smattheis commented 6 years ago

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!

Conengmo commented 6 years ago

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/

Conengmo commented 6 years ago

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.

Conengmo commented 6 years ago

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.

thomhubers commented 5 years ago

So since it's approved and everyone's happy. Can this be merged?