Skip to content

Conversation

@garthk
Copy link

@garthk garthk commented Jun 17, 2019

Work-in-progress on typespecs for the measurements and metadata. Would fix #5.

@garthk
Copy link
Author

garthk commented Jun 17, 2019

The code is a mess, but I like the results.

  @typedoc "Measurements we send via `:telemetry.execute/3`"
  @type measurements :: %{required(:count) => pos_integer(), optional(:ms) => pos_integer()}

  @typedoc "Metadata we send via `:telemetry.execute/3`"
  @type metadata :: %{required(:events) => list(Event.t()), optional(:payload) => binary()}

  defevent([:honeycomb, :start],
    measurements: measurements(),
    metadata: metadata(),
  )


ExDoc results

I think the easiest way to make this clean is to:

  • Remove the t_ from t_measurements and t_metadata
  • Remove defaults
  • Emit function_name/0 if we don't get a measurements or a metadata
  • Emit function_name/1 if we get a measurements but no metadata
  • Emit function_name/2 if we get both
  • Error out if we get metadata only

Determining the arity of the emitted function from the presence or absence
of `measurements` and `metadata` might feel like we were "forced" to use
typespecs, but `map` isn't so bad...
@garthk garthk mentioned this pull request Jun 17, 2019
@garthk
Copy link
Author

garthk commented Jun 17, 2019

Yeah, that feels better. As I say in the commit:

Determining the arity of the emitted function from the presence or of measurements and metadata might feel like we were "forced" to use typespecs, but map isn't so bad...

Reading my docs output as if I were consuming my own package, I liked how the lack of a \\ %{} made it clear to me that I'm not going to get any measurements.

Sorry for dragging #1 into this; I just noticed I'm using the :telemetry terms by default despite that not having been settled.

@garthk
Copy link
Author

garthk commented Jun 17, 2019

Meanwhile, I'm falling out of love with that defstruct pattern for options. I like how Kernel.struct!/2 blows up when the user gets a key wrong, but prefer the typings for keyword lists otherwise.

@garthk
Copy link
Author

garthk commented Jun 17, 2019

This is obsolete given your 0.2.0, no?

@coryodaniel
Copy link
Owner

I believe so, but I haven’t tested it with require/1, optional/1 on maps yet.

Sorry for the delay, been blowing through this project and a few others to tighten up some stuff to release a work project OS I’ve been working on.

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.

Support add'l typespecs

2 participants