search

open-telemetry / opentelemetry-erlang-api

Erlang/Elixir OpenTelemetry API
Apache License 2.0
60 stars 13 forks source link

Fix dialyzer issues #82

Closed davydog187 closed 3 years ago

davydog187 commented 3 years ago

Fixes nearly two dozen dialyzer issues. Need some help with the proper typespecs for a few

davydog187 commented 3 years ago

FYI that rebar3 dialyzer passes on master, while running it from mix dialyzer does not pass

davydog187 commented 3 years ago

I need to figure out the github actions caching here. Also, for some reason persistent_term:put/2 is failing on OTP 21.3 even though it should be available 🤔

davydog187 commented 3 years ago

Looks like absinthe experienced the same issue

https://elixirforum.com/t/help-with-dialyzer-and-nowarn-function/30246 https://github.com/absinthe-graphql/absinthe/pull/895/commits/445e662ca92b789c23555fe9dcb3b7307f340756

tsloughter commented 3 years ago

Huh, so the persistent_term:put only with Elixir's dialyzer call... that is really odd.

tsloughter commented 3 years ago

@davydog187 any update on this?

bryannaegele commented 3 years ago

I think we just need to add a dialyzer ignore file

On Fri, Sep 18, 2020, 5:13 PM Tristan Sloughter notifications@github.com wrote:

@davydog187 https://github.com/davydog187 any update on this?

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/open-telemetry/opentelemetry-erlang-api/pull/82#issuecomment-695123664, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABLXHVKLFOW3DSALENEFJD3SGPSQZANCNFSM4QQ6SGUA .

tsloughter commented 3 years ago

So elixir/mix doesn't have support for ignoring dialyzer warnings either in the module or in the mix config?

davydog187 commented 3 years ago

We've been working on a launch the last few weeks. I'll try to pick this back up during this week

davydog187 commented 3 years ago

Regarding suppressing the dialyzer warnings: that can be done through the .dialyzer_ignore.exs file.

I'm not 💯 on this, but that won't help dependent code.

tsloughter commented 3 years ago

I'll try moving these changes over to the new api under the opentelemetry-erlang repo https://github.com/open-telemetry/opentelemetry-erlang/tree/master/apps/opentelemetry_api

hauleth commented 3 years ago

@tsloughter git subtree pull -P apps/opentelemetry_api repo branch

tsloughter commented 3 years ago

@hauleth fails saying it is an "unrelated history", I'm guessing because of the PR having been squashed and merged.

hauleth commented 3 years ago

Well, it seems so. In that case I do not have simple way to port other than generating diff and manually applying it.

I can do it in a moment.

davydog187 commented 3 years ago

I can just close and reopen it tomorrow. Would that be better for you guys?

tsloughter commented 3 years ago

I can let you know tomorrow morning, I'll see if I can knock it out in short order today.

hauleth commented 3 years ago

I close this, as it is no longer relevant as it was ported to the open-telemetry/opentelemetry-erlang#101 and merged there.