Skip to content
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

Vendor extension rename #268

Merged
merged 6 commits into from May 2, 2019

Conversation

Projects
None yet
2 participants
@kelnos
Copy link
Member

commented Apr 30, 2019

Addresses #266

Contributing to Twilio

All third party contributors acknowledge that any contributions they provide will be made under the same open source license that the open source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

@kelnos kelnos requested a review from blast-hardcheese Apr 30, 2019

@kelnos kelnos force-pushed the kelnos:vendor-extension-rename branch 2 times, most recently from 2e33794 to 4c01901 Apr 30, 2019

extractWithFallback[F, String](v, "x-tracing-label", "x-scala-tracing-label")

def JvmPackage[F: VendorExtension.VendorExtensible](v: F): Option[String] =
extractWithFallback[F, String](v, "x-jvm-package", "x-scala-package")

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Apr 30, 2019

Collaborator

Thoughts on having x-scala-package, x-java-package, and x-jvm-package? Different aesthetics for organization by language, perhaps? Alternately, diamond dependencies on one client generated for Scala and the same one generated for Java?

This comment has been minimized.

Copy link
@kelnos

kelnos Apr 30, 2019

Author Member

Ehhhhhhhhh, does this really seem worth pre-emptively adding before someone asks for it?

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Apr 30, 2019

Collaborator

I wouldn't think twice about it other than "This is deprecated! Change to this other thing!" later "We brought it back!"

This comment has been minimized.

Copy link
@kelnos

kelnos Apr 30, 2019

Author Member

Yeah, true. If you really want it, I'll add it; just annoying because it means adding another ScalaTerm a la CustomTypePrefixes

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese May 1, 2019

Collaborator

I can't honestly say I'm comfortable merging without it. This might be an opportunity to generalize the previous one-off target-interpolating setting hierarchy?

This comment has been minimized.

Copy link
@kelnos

kelnos May 1, 2019

Author Member

Sure. Got a problem, though. JvmPackage is used in SwaggerGenerator (in GetClassName), which is conceptually lower in the stack than ScalaGenerator/JavaGenerator (which is what provides the list of language-specific prefixes to use).

One possibility I can think of is to put a call to ScalaTerm's customPrefixes() in Common.prepareDefinitions() (which is the only place that calls getClassName(), and also already has a reference to ScalaTerms), and then add a param to getClassName() to pass those prefixes in, so it can do the vendor extension extraction using the prefix list. (Looks like ServerGenerator also calls getClassName(), so that call site would have to be changed as well.)

What do you think?

This comment has been minimized.

Copy link
@kelnos

kelnos May 1, 2019

Author Member

(Ended up pushing a couple more commits to do just that. LMK if that seems reasonable.)

@kelnos kelnos force-pushed the kelnos:vendor-extension-rename branch 3 times, most recently from fb10f5e to a8dec28 May 1, 2019

@kelnos kelnos referenced this pull request May 2, 2019

Closed

Add security support for Dropwizard #274

0 of 1 task complete

kelnos added some commits Apr 30, 2019

Rename vendor extensions to be more generic
This preserves the old names for back-compat (while printing a
deprecation warning).  Changes include:

x-scala-tracing-label -> x-tracing-label
x-scala-package -> x-jvm-package
x-scala-empty-is-null -> x-empty-is-null
x-scala-file-hash -> x-file-hash
Generalize CustomTypeNames to VendorPrefixes
This way we can use these prefixes for more than just x-jvm-type etc.
Add back x-{scala,java}-package using VendorPrefixes
This gives spec writers more flexibility, but also allows for
x-jvm-package to be used when there's no need to differ between Scala
and Java.

@kelnos kelnos force-pushed the kelnos:vendor-extension-rename branch from a8dec28 to 55ed01e May 2, 2019

def ScalaType[F: VendorExtension.VendorExtensible](v: F): Option[String] =
VendorExtension(v).extract[String]("x-scala-type")
private def extractFromNames[F: VendorExtension.VendorExtensible, T: Extractable](v: F, names: List[String]): Option[T] =
names.foldLeft(Option.empty[T])(

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese May 2, 2019

Collaborator

This is actually names.collectFirstSome(VendorExtension(v).extract[T](_)), fwiw

This comment has been minimized.

Copy link
@kelnos

kelnos May 2, 2019

Author Member

Ooh, TIL. Will update and then merge.

@blast-hardcheese
Copy link
Collaborator

left a comment

lgtm!

Refactor custom type name extraction
This just 'hides' the vendor extension name construction in the extract
package object where it belongs.

@kelnos kelnos force-pushed the kelnos:vendor-extension-rename branch from 55ed01e to 75573ec May 2, 2019

@kelnos kelnos merged commit 67b94a0 into twilio:master May 2, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kelnos kelnos deleted the kelnos:vendor-extension-rename branch May 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.