Respect java_package option in protobuf files
#324
Conversation
't need to be explicit
… passing in nothing for no packages. I'm going to look into the parser more now
Codecov Report
@@ Coverage Diff @@
## master #324 +/- ##
==========================================
+ Coverage 83.93% 84.01% +0.07%
==========================================
Files 29 29
Lines 1899 1908 +9
Branches 23 27 +4
==========================================
+ Hits 1594 1603 +9
Misses 305 305
Continue to review full report at Codecov.
|
…ions but I think it's worth a code review now
dmarticus
changed the title
java_package option in protobuf filesjava_package option in protobuf files
| val fileName: String = formatName(file.getName) | ||
| // This package naming behavior is consistent with ScalaPB, see | ||
| // https://scalapb.github.io/docs/generated-code/#default-package-structure | ||
| if (javaPackage.isEmpty) { |
There was a problem hiding this comment.
Quick note here: I implemented this method to stay consistent with the parsing behavior of ScalaPB. If we want to make it so that we don’t use the filename on the absence of any package information (which would fail the parsing step), I’m open to that as well. At this point, without better direction, I'm just going for compatibility with an existing solution.
| // header (which will just return the name of the type). This method trims the type name from a fully- | ||
| // qualified name if necessary, which provides consistent behavior when comparing the name of a message | ||
| // to the name of a type | ||
| fullName.substring(fullName.lastIndexOf(".") + 1) |
There was a problem hiding this comment.
Another long comment that basically explains the caveat here: we no longer will be comparing fully-qualified type names to the type that appears in the method, because getTypeName behaves differently depending on what it's called against (if called against a file with a package header, it returns the fully-qualified type, whereas if called against a file with a java_package header, it just returns the typeName). In this case, we always want to just use the typename, so we parse the string down if necessary. Yes, I know it's ugly "strings as types" but when working with the java methods that only return strings there's not a lot I can do about it :|
| @@ -1,6 +1,6 @@ | |||
| syntax = "proto3"; | |||
|
|
|||
| package com.acme; | |||
| option java_package = "com.acme"; | |||
There was a problem hiding this comment.
testing that the java_package interop works seamlessly when importing proto files (spoiler: it does)
| @@ -25,7 +25,7 @@ import org.apache.avro.{Schema, Protocol => AvroProtocol} | |||
| import higherkindness.droste._ | |||
| import higherkindness.droste.syntax.all._ | |||
|
|
|||
| import scala.jdk.CollectionConverters._ | |||
| import scala.collection.JavaConverters._ | |||
There was a problem hiding this comment.
fwiw the explanation on why I'm making this change is nicely laid out by Birchall here: #322 (comment)
Closes #318. Includes test cases provided by the issue filer :), plus some additional ones that cover missing package info altogether.
This PR is intended to give Skeuomorph comparable behavior to ScalaPB when it comes to handling the package structure -- I want to make it so that the package hierarchy goes like this (1) java package (if present) (2) package name (if present) (3) file name. More on how ScalaPB does it here here.