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

Better error messages #402

Merged
merged 30 commits into from Oct 3, 2019

Conversation

blast-hardcheese
Copy link
Member

@blast-hardcheese blast-hardcheese commented Sep 20, 2019

Problem:

Missing operationId is not a useful error message, especially with particularly large specification files.

--

Given:

class Foo(val a: Bar)
class Bar(val b: Baz)
class Baz(val c: String)

the following will compile:

val result: Tracker[Option[String]] =
  Tracker(foo).downField("b", _.b).flatDownField("c", _.c)

there are a number of combinators borne of necessity, though my intent that it should be possible to write Traverse and Monoid instances for these structures, which may simplify syntax a bit.

I've broken out IndexedFunctor and IndexedDistributive, as well as some syntax around these combinators, but that permitted a significant reduction in bespoke syntax extensions.

The intent of opening this PR is to get feedback on this approach, as well as to publicize the fact that this is coming, in case others plan on doing major refactors.

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.

@blast-hardcheese blast-hardcheese changed the title Tracker Better error messages Sep 20, 2019
@blast-hardcheese
Copy link
Member Author

OK, ready for review. There are many more warnings around .get, but this is to track the process of infecting more functions with Tracker. Everywhere there's a .get currently, there's an opportunity to provide better error messages, as we're unwrapping the Tracker directly without showing its history to the user.

} yield res
)
.orRefine({ case ref: Schema[_] if Option(ref.get$ref).isDefined => ref })(ref => getSimpleRef(ref.get).map(Deferred[L]))
.orRefine({ case b: BooleanSchema => b })(buildResolve(litBoolean))
Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out these were all the same, with only two variants (has default vs no default possible)

(" " * indent) + value.toString().linesIterator.filterNot(_.contains(": null")).mkString("\n" + (" " * indent))
}

class RichCollection[A, Repr](xs: IterableLike[A, Repr]) {
Copy link
Member Author

Choose a reason for hiding this comment

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

from https://stackoverflow.com/a/3912833. distinctBy is a member of 2.13's View, so this can go away when we upgrade.

Copy link
Member

@kelnos kelnos left a comment

Choose a reason for hiding this comment

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

This looks mostly fine to me. I don't really understand some or it, like IndexedDistributive, but I appreciate that there are new tests around this, and all the old tests still pass.

implicit class OptionSyntax[A](tracker: Tracker[Option[A]]) {
def orHistory: Either[Vector[String], Tracker[A]] = tracker.indexedCosequence.toRight(tracker.history)
def raiseErrorIfEmpty(err: String): Target[Tracker[A]] = Target.fromOption(tracker.indexedCosequence, s"${err} (${tracker.showHistory})")
class FlatDownFieldPartiallyApplied[C](val dummy: Boolean = true) {
Copy link
Member

Choose a reason for hiding this comment

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

extends AnyVal? (Otherwise, why bother with dummy?)

(& other examples of the same in this file)

Copy link
Member Author

Choose a reason for hiding this comment

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

This follows the pattern used in cats to partially apply type arguments. dummy is presumably required to avoid syntactic ambiguity, but the big thing is by fixing C in the call site, it permits:

t.downField[Int]("foo", _.getFoo) instead of t.downField[Option[Int], Int]("foo", _.getFoo)

Small improvement in ergonomics for the user, as it frees the unspecified type parameter to be resolved by the compiler.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I've seen that pattern cats, but they do extends AnyVal on those. I assumed the need for dummy was just because you have to have one-and-only-one val constructor param when extending AnyVal.

@blast-hardcheese
Copy link
Member Author

blast-hardcheese commented Oct 3, 2019

To follow up on "I don't really understand some of it, like IndexedDistributive", I found Distributive to solve part of the problem I was having, as described here:

given G[F[A]], I want F[G[A]], but I want the typeclass that controls exactly how G[_] is applied over the different F[_]s to know that that is what's happening, instead of how traverse/sequence work, in the sense of strictly regimented generic methods that don't know about either structure.

I think the name comes from the distributive property of arithmetic: A * (B + C) == (A * B) + (A * C), Tracker * List[B + C] == List[Tracker[B] + Tracker[C]].


The other side of the name, Indexed, comes from the fact that Distributive in that Hackage link doesn't know anything about the implementation details of the data structure it's walking over. I needed the key: String of a Map to be in scope so I could appropriately represent that we've indexed into a Tracker[Map[K, V]] to get a Map[K, Tracker[V]] for all Ks, similarly with Tracker[List[A]] and List[Tracker[A]]. This is expressed in the tests by the history.downField.list and history.downField.map tests.


Initially I actually didn't have this abstraction, and I ended up writing almost identical syntax extensions for all the different structures I was trying to represent. There was a lot of duplication and divergent method names, since I wasn't sure what I had already implemented. When I started unifying them under this new banner of IndexedFunctor and IndexedDistributive things really started cleaning up.

@blast-hardcheese
Copy link
Member Author

Rebased!

@kelnos
Copy link
Member

kelnos commented Oct 3, 2019

Welp, I don't really understand the stuff in that hackage link, but your description helped a lot, thanks!

@blast-hardcheese blast-hardcheese merged commit 397621c into guardrail-dev:master Oct 3, 2019
@blast-hardcheese blast-hardcheese deleted the tracker branch October 3, 2019 20:57
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.

None yet

2 participants