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

add scala-styled lambdas rendering #373

Merged

Conversation

Ivoyaa
Copy link
Contributor

@Ivoyaa Ivoyaa commented Feb 6, 2023

No description provided.

@CLAassistant
Copy link

CLAassistant commented Feb 6, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@pshirshov pshirshov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another thing which is still missing. Currently we format prefixes and names in our own non-scala notation (there are some additional symbols, etc) we should take care of that aspect too.

override implicit lazy val r_TypeParam: Renderable[TypeParam] = new Renderable[TypeParam] {
override def render(value: TypeParam): String =
value.ref match {
case n: NameReference if n.symName.name.forall(_.isDigit) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a heuristic which may fail. We need to preserve the context here and check the names against it. Also we might want to add more metadata into the model to simplify the logic here but that's undesirable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added LambdaParamNameMaker to hold the logic for both creating param names and checking if a string is a param name in one place. Doesn't seem to me like a clean solution but can't think of any others except extra metadata in model

@Ivoyaa Ivoyaa force-pushed the wip/367-add-scala-styled-lambda-rendering branch from 4a1e30a to f4ad8ef Compare February 15, 2023 22:34
@pshirshov
Copy link
Member

pshirshov commented Feb 18, 2023

This seems to be working, though it still looks like an ad-hoc solution.

I would try one of two options:

  1. We might add metadata into the model, why not. Most likely that should be a flag in NameReference or (better) a separate branch LambdaParam in SymName.
  2. Instead of using implicit resolution in LTTRenderables we might write a recursive traverser which would preserve the context on the stack. I think this solution should be much easier to implement (and it won't break bincompat).

…a-styled-lambda-rendering

# Conflicts:
#	izumi-reflect/izumi-reflect/src/main/scala-2/izumi/reflect/macrortti/LightTypeTagImpl.scala
#	izumi-reflect/izumi-reflect/src/main/scala-3/izumi/reflect/TagMacro.scala
#	izumi-reflect/izumi-reflect/src/main/scala/izumi/reflect/macrortti/LightTypeTagRef.scala
@Ivoyaa Ivoyaa force-pushed the wip/367-add-scala-styled-lambda-rendering branch from f1b25c9 to ac508a4 Compare March 8, 2023 21:32
@Ivoyaa Ivoyaa force-pushed the wip/367-add-scala-styled-lambda-rendering branch from ac508a4 to a811bbe Compare March 8, 2023 21:56
override def render(value: Boundaries): String = value match {
case Boundaries.Defined(bottom, top) =>
(bottom.render(), top.render()) match {
case (bottomRendered, topRendered) if bottomRendered == "scala.Nothing" => s"<: $topRendered"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kinda dirty. We already have all the necessary constants in LightTypeTagInheritance, please reuse them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, removed my hardcode

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants