-
Notifications
You must be signed in to change notification settings - Fork 15
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
Extend package choosing behavior to protobuf files with Enum
types
#339
Conversation
…st need a new perspective on them lol
…. Now I need to update the name extracting method to not fail on fully-qualified names that end with the same word
…returned by the fully-qualified type
Codecov Report
@@ Coverage Diff @@
## master #339 +/- ##
==========================================
+ Coverage 85.20% 85.23% +0.02%
==========================================
Files 29 29
Lines 1954 1951 -3
Branches 28 24 -4
==========================================
- Hits 1665 1663 -2
+ Misses 289 288 -1
Continue to review full report at Codecov.
|
src/main/scala/higherkindness/skeuomorph/protobuf/ParseProto.scala
Outdated
Show resolved
Hide resolved
@BenFradet let me know what you think of my explanation for your question when you have time :) |
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.
(See my reply to the comment thread.)
…to commit a good state though so I can happily break things going forward
src/main/scala/higherkindness/skeuomorph/protobuf/ParseProto.scala
Outdated
Show resolved
Hide resolved
src/main/scala/higherkindness/skeuomorph/protobuf/ParseProto.scala
Outdated
Show resolved
Hide resolved
src/main/scala/higherkindness/skeuomorph/protobuf/ParseProto.scala
Outdated
Show resolved
Hide resolved
package
handling to protobuf files with Enum
typesEnum
types
src/main/scala/higherkindness/skeuomorph/protobuf/ParseProto.scala
Outdated
Show resolved
Hide resolved
@@ -450,17 +443,22 @@ object ParseProto { | |||
} | |||
} | |||
|
|||
private def matchOnPackageParameters[A](name: String, file: NamedDescriptor[A]) = { |
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 suggest integrating this method into the initializer of NamedDescriptor.fullName
(which isn't directly used anywhere else) and then simply calling file.fullName.endsWith(name)
.
Perhaps we could also change fullName
to a val
and rename it to something more descriptive like fullProtoName
.
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.
Following up on that - we could have the following implementations in NamedDescriptor
:
val fullProtoName: String =
(List(".", filePackage) ++ parentMessageNames :+ name).mkString(".")
val scalaPrefix: List[String] =
(scalaPackage.split('.').toList :+ enclosingProto) ++ parentMessageNames
I don't know if it would work for all cases, but it passes the unit tests at least. Also it means we could remove the javaPackage
parameter and fqnParts
member.
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 like this idea a lot! Thanks for sharing! I will take a crack at this implementation :)
…cala Co-authored-by: Lawrence Lavigne <contact.lavigne@gmail.com>
@@ -121,7 +123,7 @@ object ParseProto { | |||
} | |||
} | |||
|
|||
private def getPackageOrJavaPackage(file: FileDescriptorProto): String = { | |||
private def namePackage(file: FileDescriptorProto): String = { |
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.
(thinking aloud) should this be called choosePackageFrom
or something else? namePackage
seems ambiguous in a way I don't like.
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 think we should call it scalaPackage
since that's what the ScalaPB doc (linked in the method's comment) says it is. We'd also rename the NamedDescriptor
parameter accordingly. (As per my other suggestion: fqn
would become scalaPackage
and packageName
would become filePackage
).
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 addressed this comment and your other one in my most recent change! @L-Lavigne :)
@L-Lavigne any insights as to why this dropped coverage? I added more tests than before but the code coverage bot says I decreased coverage overall... might add some additional unit tests to see if that helps. |
* a `java_package` and a `package`, so this approach makes sure that we always match with the correct | ||
* value). | ||
*/ | ||
val fullName: String = |
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 don't think we need this one anymore, in my experiments I was matching on fullProtoName
instead (since that one is specially built using the filePackage
and not the javaPackage
). If that works we can eliminate the javaPackage
constructor parameter altogether.
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.
oh yeah good call! Sorry; thank you for clarifying :)
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.
Great job! 🎉
Closes #337.
cc @a7emenov