-
Notifications
You must be signed in to change notification settings - Fork 399
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
[gen] fix codegen to reference subtypes nested with their encapsulating object when referenced directly #2869
[gen] fix codegen to reference subtypes nested with their encapsulating object when referenced directly #2869
Conversation
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 have time rn to take a deeper look and think about implications. But please try to simplify the code. I think there is too much generalisation where it might not be needed.
// The following function will be used to alter the type of all fields needed. | ||
// The `mapCaseClasses` helper takes a function that alters a case class, | ||
// and lifts it such that we can apply it to any structure, and it'll take care to recurse when needed. | ||
val mapType: String => Code.ScalaType => Code.ScalaType = encapsulatingName => |
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.
Too complicated. Can be a def (st: Code.ScalaType): ScalaType
, since the string is always the scala type name.
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.
done
@@ -39,6 +39,12 @@ object EndpointGen { | |||
case None => m - k | |||
} | |||
} | |||
|
|||
implicit class OptionCompanionCompatOps(o: Option.type) { |
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.
You use this here only once. Please inline. Makes the code to complicated
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.
done
* @return | ||
* [[Option]] of the altered type, or None if no modification was needed. | ||
*/ | ||
def mapTypeRef(sType: Code.ScalaType)(f: Code.TypeRef => Option[Code.TypeRef]): Option[Code.ScalaType] = |
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 have so much time to take a deep look, but can this be simplified to return non option and always use the result, even if it is the previous value. Efficiency is not so important for code gen. We should prefer maintainability.
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.
simplified
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.
simplified - no options
* @param f | ||
* a function that may alter the type, None means no altering is needed. | ||
* @return | ||
* [[Option]] of the altered type, or None if no modification was needed. |
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.
use full path to fix doc generation
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.
no need, Option is no longer used
* structure recursively. | ||
* | ||
* @param f | ||
* function to transform a [[Code.CaseClass]] |
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.
use full path to fix doc generation
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.
done
fixes #2868
This code will:
Animal.Zebra
as in the added test)Animal
object, does not need to prependAnimal.
to any subtypes referenced directly)Option[Animal.Alligator]
as in the added tests