wdtinc / mapbox-vector-tile-java

Java Mapbox Vector Tile Library for Encoding/Decoding
Apache License 2.0
147 stars 73 forks source link

Replace ProtocolStringList with List<String> #12

Closed djohnson729 closed 6 years ago

djohnson729 commented 6 years ago
ShibaBandit commented 6 years ago

Hey @djohnson729 , thank you for the pull request. Tests are passing. Is the protobuf 2.5 compatibility solved by removing that interface reference? I can't guarantee this will work without using the same protobuf implementation as what was used to generate the com.wdtinc.mapbox_vector_tile.VectorTile class. In #11 I recommended using a tool like 'shade' to help with dependency conflicts.

djohnson729 commented 6 years ago

@ShibaBandit I'll take a look at that issue. I hadn't seen that. What I did myself was to copy your code into my own project (of course keeping all of your copyright info in the source headers and adding a readme to give proper attribution), make the mods I included in the pull request, and then I also added the following to my POM to run protoc as part of maven. So that ensures the protoc version used to generate VectorTile.java matches the protobuf version used.

      <plugin>
        <groupId>org.xolstice.maven.plugins</groupId>
        <artifactId>protobuf-maven-plugin</artifactId>
        <version>0.5.1</version>
        <configuration>
          <!--
          <protocExecutable>/usr/local/bin/protoc</protocExecutable>
          -->
          <protoSourceRoot>${project.basedir}/src/main/resources</protoSourceRoot>
          <outputDirectory>${project.build.directory}/generated-sources/protobuf/java</outputDirectory>
        </configuration>
        <executions>
          <execution>
            <goals>
              <goal>compile</goal>
              <goal>test-compile</goal>
            </goals>
            <configuration>
              <protocArtifact>com.google.protobuf:protoc:2.5.0:exe:${protoc.artifact.system}</protocArtifact>
            </configuration>
          </execution>
        </executions>
      </plugin>
ShibaBandit commented 6 years ago

Interesting, so your solution regenerates the protobuf bindings?

As far as I can tell from inspecting the unit tests this is good and List is probably a cleaner interface so I'll go ahead and approve.

Thanks again for the PR.

ShibaBandit commented 6 years ago

Let me know if you need a release, but I'm guessing since your running the protobuf-maven-plugin on the source that may not be needed.

djohnson729 commented 6 years ago

Yes, that's right. It generates the VectorTile.java every time you build. And it might make sense to include a property in your POM file for the protobuf version so that you can override it when you build. But the downside to that is that you would not be able to determine from the artifact which protobuf version it was built against.

I won't need a new release because I just copied all of your code (including the changes I submitted to you) along with attribution so I could deal with it the way I needed to. I just wanted to send the changes back to you for the benefit of the community. Thanks for taking a look!