-
-
Notifications
You must be signed in to change notification settings - Fork 113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move basemap profile into OpenMapTiles submodule #258
Conversation
https://github.com/onthegomap/planetiler/actions/runs/2759982874 ℹ️ Base Logs 8a8db00
ℹ️ This Branch Logs 3899452
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments. Also looks like my invite to the planetiler-openmaptiles repo expired, would you mind sending over another one?
} | ||
return name.trim(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything above here has been reimplemented from scratch, let's move that (and corresponding tests) to a LanguageUtils
class in the planetiler core module so the only openmaptiles-specific stuff that gets relicensed is the specific logic for mapping to output keys in getNames
and getNamesWithoutTranslations
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this code to planetiler/util/LanguageUtils.java and planetiler/util/Utils.java.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR to planetiler-openmaptiles
created openmaptiles/planetiler-openmaptiles#9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting sonar failed message here because of missing test coverage. I kept the original tests in planetiler-openmaptiles
LanguageUtilsTest as they all use getNames
method. If I understand correctly this is original omt-tools method and thus should be in planetiler-openmaptiles
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just pushed a few updates to this and a new version of LanguageUtilsTest
@@ -83,7 +83,7 @@ | |||
|
|||
<modules> | |||
<module>planetiler-core</module> | |||
<module>planetiler-basemap</module> | |||
<module>planetiler-openmaptiles</module> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you'll want to build/run the planetiler-openmaptiles on its own from the standalone repo? If so we'll want to think through how we are going to structure the pom.xml's so it can be run both standalone and as a child project here.
One option is what the planetiler-examples submodule does - it has 2 pom.xml's, one for standalone and one for a child module (although we could rename so main pom.xml is for standalone). We'll need to keep the 2 in sync but shouldn't be too bad unless you start adding a lot of dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not think about it. For now, I build/run it from the parent planetiler repo, even for GH actions.
I am open to change it, I think having two pom.xml in sync could work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, sounds good for now - I could help set that up later.
@msbarry I sent you another invite, let me know if you have an issue accessing the repo. |
} | ||
return name.trim(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just pushed a few updates to this and a new version of LanguageUtilsTest
SonarCloud Quality Gate failed. |
planetiler-core/src/main/java/com/onthegomap/planetiler/util/Utils.java
Outdated
Show resolved
Hide resolved
…submodule to READMe
Kudos, SonarCloud Quality Gate passed! |
This PR moves the basemap profile into a separate repository planetiler-openmaptiles under the openmaptiles org as discussed in #228.
The aim is to separate the code of Planetiler (the tool) from OpenMapTiles schema. The reasons and benefits summarised in the discussion #228 (comment)
The code of the planetiler-openmaptiles is currently up-to-date with planetiler root repo. I released v3.13 and v3.13.1 to keep the same versioning as in OMT
How to use :