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

Enable lazy construction of label strings #1041

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Mar 16, 2024

A different take on #979

Instead of recycling |: for labeling, use =|= for improved symmetry and operator precedence.

A second commit follows up scalafmt config by "fixing" star alignment in doc comments. Edit: That hassle is reserved for another effort.

@som-snytt
Copy link
Contributor Author

som-snytt commented Mar 16, 2024

TIL scalafmt doesn't do headers.

Edit: dropped the scalafmt for further research

image

One hint is to scalafmtAll every commit. Edit: make that +scalafmtAll.

@som-snytt som-snytt force-pushed the topic/by-name-label branch 3 times, most recently from 97e168c to 818097a Compare March 16, 2024 04:31
@som-snytt som-snytt force-pushed the topic/by-name-label branch from 818097a to 728710e Compare March 16, 2024 04:35
@@ -583,6 +583,8 @@ object GenSpecification extends Properties("Gen") with GenSpecificationVersionSp
}
val avg = sum / n
s"average = $avg" |: avg >= 0.49 && avg <= 0.51
s"average = $avg".ensuring(false, "eager evaluation") =|= avg >= 0.49 && avg <= 0.51
avg >= 0.49 && avg <= 0.51 =|= s"average = $avg"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just to exercise the idioms in the absence of tests

@som-snytt som-snytt marked this pull request as ready for review March 16, 2024 05:28
@som-snytt
Copy link
Contributor Author

On precedence, the trade-off is the && combinator for Prop (as opposed to the boolean op in an expression promoted to Prop).

* The label is evaluated lazily. The operator name is chosen for its precedence btween boolean operators and
* others.
*/
def =|=(label: => String): Prop = prop.map(_.label(label))

Choose a reason for hiding this comment

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

Another part of my PR (#979) was to avoid evaluating the label if it will never be displayed to the user. What do you think about doing the same here?

def =|=(label: => String): Prop =
  prop.map(r =>
    r.status match {
      case Prop.False | Prop.Exception(_) => r.label(l)
      case Prop.Proof | Prop.True | Prop.Undecided => r
    })

*
* The operator name is chosen for its precedence between boolean operators and others.
*/
extension (prop: Prop) def =|=(label: => String): Prop = prop.map(_.label(label))

Choose a reason for hiding this comment

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

Does Scala 3 not need an extension for Boolean like the Scala 2 version?

@mrdziuban
Copy link

@satorg @som-snytt do you have any thoughts on moving this or #979 forward?

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.

2 participants