Extend package choosing behavior to protobuf files with `Enum` types #339

dmarticus · 2020-09-28T23:44:57Z

Closes #337.

cc @a7emenov

…dylan bound

…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 · 2020-09-28T23:51:50Z

Codecov Report

Merging #339 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            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

Impacted Files	Coverage Δ
...igherkindness/skeuomorph/protobuf/ParseProto.scala	`96.57% <100.00%> (-0.06%)`	⬇️
...in/scala/higherkindness/skeuomorph/mu/schema.scala	`65.33% <0.00%> (+1.33%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 574226d...e558de3. Read the comment docs.

src/main/scala/higherkindness/skeuomorph/protobuf/ParseProto.scala

dmarticus · 2020-10-05T18:34:47Z

@BenFradet let me know what you think of my explanation for your question when you have time :)

L-Lavigne

(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

L-Lavigne · 2020-10-23T06:03:49Z

src/main/scala/higherkindness/skeuomorph/protobuf/ParseProto.scala

@@ -450,17 +443,22 @@ object ParseProto {
    }
  }

+  private def matchOnPackageParameters[A](name: String, file: NamedDescriptor[A]) = {


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.

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.

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>

dmarticus · 2020-10-23T04:39:46Z

src/main/scala/higherkindness/skeuomorph/protobuf/ParseProto.scala

@@ -121,7 +123,7 @@ object ParseProto {
    }
  }

-  private def getPackageOrJavaPackage(file: FileDescriptorProto): String = {
+  private def namePackage(file: FileDescriptorProto): String = {


(thinking aloud) should this be called choosePackageFrom or something else? namePackage seems ambiguous in a way I don't like.

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).

I addressed this comment and your other one in my most recent change! @L-Lavigne :)

dmarticus · 2020-10-23T23:31:17Z

@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.

L-Lavigne · 2020-10-24T00:25:12Z

src/main/scala/higherkindness/skeuomorph/protobuf/ParseProto.scala

+     * a `java_package` and a `package`, so this approach makes sure that we always match with the correct
+     * value).
+     */
+    val fullName: String =


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.

oh yeah good call! Sorry; thank you for clarifying :)

L-Lavigne

Great job! 🎉

dmartin-TA and others added 4 commits September 27, 2020 15:04

making my change downstream, walking fast, faces past and I'm /Users/…

cfdc57e

…dylan bound

there's no reason these tests don't work but they still don't so I ju…

b3b96e8

…st need a new perspective on them lol

test bed is created and my ideal behavior for a simple scenario works…

4b00c1e

…. Now I need to update the name extracting method to not fail on fully-qualified names that end with the same word

make it so that we trim the type name based on the amount of nesting …

d85e0be

…returned by the fully-qualified type

dmarticus requested review from cb372, BenFradet, juanpedromoreno, dzanot and L-Lavigne September 28, 2020 23:44

BenFradet reviewed Sep 29, 2020

View reviewed changes

src/main/scala/higherkindness/skeuomorph/protobuf/ParseProto.scala Outdated Show resolved Hide resolved

dmartin-TA added 2 commits September 29, 2020 12:11

added a comment to the method

316151f

forgot to format

ff78c78

dmarticus requested a review from BenFradet September 30, 2020 14:31

dzanot previously approved these changes Oct 5, 2020

View reviewed changes

L-Lavigne reviewed Oct 5, 2020

View reviewed changes

renaming the method to make it more explicit

eceff15

dmarticus dismissed dzanot’s stale review via eceff15 October 5, 2020 22:07

Merge branch 'master' into issue337

a39ceac

dmarticus requested review from L-Lavigne and dzanot October 6, 2020 00:23

dmartin-TA and others added 6 commits October 7, 2020 14:26

pushing a broken version to test a thing

77d2200

Merge branch 'master' into issue337

b02764b

Merge branch 'master' into issue337

5cbb790

this for sure works but it still doesn't leverage endsWith... I want …

f7ecbc6

…to commit a good state though so I can happily break things going forward

slightly betterer formatting

70df7d2

adding contextual content

419e97e

dzanot previously approved these changes Oct 17, 2020

View reviewed changes

L-Lavigne reviewed Oct 17, 2020

View reviewed changes

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

out with the string modifications, in with comparing packages

e7d6261

dmarticus dismissed dzanot’s stale review via e7d6261 October 22, 2020 23:36

Merge branch 'master' into issue337

3d12d9b

dmarticus requested review from L-Lavigne and dzanot October 22, 2020 23:48

L-Lavigne reviewed Oct 23, 2020

View reviewed changes

src/main/scala/higherkindness/skeuomorph/protobuf/ParseProto.scala Outdated Show resolved Hide resolved

dmarticus changed the title ~~Extend empty package handling to protobuf files with Enum types~~ Extend package choosing behavior to protobuf files with Enum types Oct 23, 2020

L-Lavigne reviewed Oct 23, 2020

View reviewed changes

src/main/scala/higherkindness/skeuomorph/protobuf/ParseProto.scala Outdated Show resolved Hide resolved

L-Lavigne reviewed Oct 23, 2020

View reviewed changes

Update src/main/scala/higherkindness/skeuomorph/protobuf/ParseProto.s…

c0522c5

…cala Co-authored-by: Lawrence Lavigne <contact.lavigne@gmail.com>

dmarticus commented Oct 23, 2020

View reviewed changes

this should do it!

d0e0a74

dmarticus requested a review from L-Lavigne October 23, 2020 23:06

attempting to placate the all-powerful codecov bot

dbafb2d

L-Lavigne reviewed Oct 24, 2020

View reviewed changes

yeah bb that did it

e558de3

dmarticus requested a review from L-Lavigne October 24, 2020 03:41

L-Lavigne approved these changes Oct 24, 2020

View reviewed changes

dmarticus merged commit 24ba7e7 into master Oct 24, 2020

dmarticus deleted the issue337 branch October 24, 2020 04:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend package choosing behavior to protobuf files with `Enum` types #339

Extend package choosing behavior to protobuf files with `Enum` types #339

dmarticus commented Sep 28, 2020

codecov bot commented Sep 28, 2020 •

edited

dmarticus commented Oct 5, 2020

L-Lavigne left a comment

L-Lavigne Oct 23, 2020 •

edited

L-Lavigne Oct 23, 2020 •

edited

dmarticus Oct 23, 2020

dmarticus Oct 23, 2020

L-Lavigne Oct 23, 2020

dmarticus Oct 23, 2020

dmarticus commented Oct 23, 2020

L-Lavigne Oct 24, 2020

dmarticus Oct 24, 2020

L-Lavigne left a comment

Extend package choosing behavior to protobuf files with Enum types #339

Extend package choosing behavior to protobuf files with Enum types #339

Conversation

dmarticus commented Sep 28, 2020

codecov bot commented Sep 28, 2020 • edited

Codecov Report

dmarticus commented Oct 5, 2020

L-Lavigne left a comment

Choose a reason for hiding this comment

L-Lavigne Oct 23, 2020 • edited

Choose a reason for hiding this comment

L-Lavigne Oct 23, 2020 • edited

Choose a reason for hiding this comment

dmarticus Oct 23, 2020

Choose a reason for hiding this comment

dmarticus Oct 23, 2020

Choose a reason for hiding this comment

L-Lavigne Oct 23, 2020

Choose a reason for hiding this comment

dmarticus Oct 23, 2020

Choose a reason for hiding this comment

dmarticus commented Oct 23, 2020

L-Lavigne Oct 24, 2020

Choose a reason for hiding this comment

dmarticus Oct 24, 2020

Choose a reason for hiding this comment

L-Lavigne left a comment

Choose a reason for hiding this comment

Extend package choosing behavior to protobuf files with `Enum` types #339

Extend package choosing behavior to protobuf files with `Enum` types #339

codecov bot commented Sep 28, 2020 •

edited

L-Lavigne Oct 23, 2020 •

edited

L-Lavigne Oct 23, 2020 •

edited