Fix Dialyzer issues #4
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
bblaszkow06
requested changes
Sep 6, 2022
mix.exs
Outdated
| @@ -52,6 +52,7 @@ defmodule Membrane.TelemetryMetrics.Mixfile do | |||
|
|
|||
| if System.get_env("CI") == "true" do | |||
| # Store PLTs in cacheable directory for CI | |||
| File.mkdir_p!(Path.join([__DIR__, "priv", "plts"])) | |||
There was a problem hiding this comment.
This should not be needed, it's part of CI pipeline
lib/telemetry_metrics.ex
Outdated
| @@ -9,6 +9,8 @@ defmodule Membrane.TelemetryMetrics do | |||
|
|
|||
| @type label() :: list() | |||
|
|
|||
| @dialyzer [{:nowarn_function, emit_event?: 1}] | |||
There was a problem hiding this comment.
- It would be best if you narrowed down the
:nowarnto a flag disabling the specific warning - Such a thing requires a comment explaining why is this disabled
bblaszkow06
reviewed
Sep 6, 2022
lib/telemetry_metrics.ex
Outdated
Comment on lines
12
to
14
| # Suppresses Dialyzer false-positive error occurring due to fact, that function contains match on module attribute known at compile time but can be overridden in the config file. | ||
| # Dialyzer returns an error saying, that pattern can never match the type, but it might be not true, in the case when somebody will change its value in the custom config file. | ||
| @dialyzer [{:no_match, emit_event?: 1}] |
There was a problem hiding this comment.
This description is way too verbose, but it can be removed altogether by simply fixing the error. Instead of defining a condition inside helper function, conditionally define the function and it seems to work, ensure it won't generate warnings in code using the macros here:
cond do
not @enabled -> defp emit_event?(_event), do: false
@events == :all -> defp emit_event?(_event), do: true
is_list(@events) -> defp emit_event?(event), do: event in @events
true -> defp emit_event?(_event), do: false
end
FelonEkonom
force-pushed
the
fix-dialyzer-issues
branch
from
b21ba86 to
f0e208a
Compare
bblaszkow06
requested changes
Sep 9, 2022
lib/telemetry_metrics.ex
Outdated
Comment on lines
87
to
91
| cond do | ||
| not @enabled -> defp emit_event?(_event), do: false | ||
| @events == :all -> defp emit_event?(_event), do: true | ||
| is_list(@events) -> defp emit_event?(event), do: event in @events | ||
| true -> defp emit_event?(_event), do: false |
There was a problem hiding this comment.
I've told you to check for warnings 😛
When MIX_ENV=test it's fine (thus CI passes) but in dev when @enabled is false, there's a warning about unused module attribute.
I've found a way to solve it - use variables for code executed at compile time:
Suggested change
| cond do | |
| not @enabled -> defp emit_event?(_event), do: false | |
| @events == :all -> defp emit_event?(_event), do: true | |
| is_list(@events) -> defp emit_event?(event), do: event in @events | |
| true -> defp emit_event?(_event), do: false | |
| enabled = Application.compile_env(:membrane_telemetry_metrics, :enabled, false) | |
| events = Application.compile_env(:membrane_telemetry_metrics, :events, :all) | |
| cond do | |
| not enabled -> defp emit_event?(_event), do: false | |
| events == :all -> defp emit_event?(_event), do: true | |
| is_list(events) -> defp emit_event?(event), do: event in events | |
| true -> defp emit_event?(_event), do: false |
bblaszkow06
approved these changes
Sep 22, 2022
lib/telemetry_metrics.ex
Outdated
Comment on lines
7
to
8
| # @enabled Application.compile_env(:membrane_telemetry_metrics, :enabled, false) | ||
| # @events Application.compile_env(:membrane_telemetry_metrics, :events, :all) |
There was a problem hiding this comment.
Suggested change
| # @enabled Application.compile_env(:membrane_telemetry_metrics, :enabled, false) | |
| # @events Application.compile_env(:membrane_telemetry_metrics, :events, :all) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related ticket on Jira board: https://membraneframework.atlassian.net/browse/MS-217?atlOrigin=eyJpIjoiOWE5ZjA3MmIxMGIyNGYxM2E2ZmMzZjQ2OGFhMWI5ZTciLCJwIjoiaiJ9