-
Notifications
You must be signed in to change notification settings - Fork 530
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
Add dialyxir to CI #1248
Add dialyxir to CI #1248
Conversation
This way it works both locally and in CI (GitHub)
This seems to vary as per Erlang version, so I took the easy way out :)
{"lib/absinthe/middleware/async.ex", :unknown_function, 117}, | ||
{"lib/absinthe/middleware/batch.ex", :unknown_function, 213}, | ||
{"lib/absinthe/utils/render.ex", :improper_list_constr, 51}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lemme know if you prefer a broader "no-line" entry.
@stopped_at_token_limit ":stopped_at_token_limit" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because nimble_parsec
specific errors as String.t()
.
Another solution is to pull request there so the error reason can be term()
, for example.
@@ -30,7 +30,8 @@ defmodule Absinthe.Mixfile do | |||
], | |||
deps: deps(), | |||
dialyzer: [ | |||
plt_core_path: "priv/plts", | |||
plt_add_deps: :apps_direct, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For lolspeed 😄
All changes to mix.lock are due to mix deps.update ex_doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, Thanks!
@paulo-ferraz-oliveira I ended up removing dialyxer from the CI in #1253 since it immediately had issues w/ PLT caches that I didn't understand. You're welcome to reintroduce it but some guidance about how to manage the caches when adding Elixir / OTP versions would be appreciated. |
Caches usually should depend on whatever would invalidate them (in terms of key naming), as in
(OS + OTP version + Elixir version + mix.lock hash) What problems did you run into? As for guidance, I think the above should be enough, but maybe you want something different (?) |
You can see the job that failed here https://github.com/absinthe-graphql/absinthe/actions/runs/5408206586/jobs/9827039687 |
Edit: it's not cache -related, but Elixir 1.15 -related.
|
Actually so if you look at this job here https://github.com/absinthe-graphql/absinthe/actions/runs/5423460943/jobs/9861420841 I added a cache key prefix and if you look at the cache action you can see it didn't find any PLT cache, but it still failed. |
Yeah, I see that. Strange that it's complaining about a missing function: https://github.com/absinthe-graphql/absinthe/actions/runs/5423460943/jobs/9861420841#step:10:18. The function is valid, though, as per https://www.erlang.org/doc/man/prettypr.html#text-1, but maybe it's not added to the analysis scope for Lemme try to look at this closer. I need to fork, pull your branch, and make it fail before I can figure out what needs fixing :) |
Yeah, so I also got it failing here: https://github.com/paulo-ferraz-oliveira/absinthe/actions/runs/5425961530/jobs/9867389890 Now on to figure stuff out 😄 Edit: will also be sure to use Erlang 25.3.2.3 and Elixir 1.15.1, locally, to make sure no issue is apparent from that distinction. |
Not breaking using Elixir 1.14.5, but breaking using 1.15.1. IIrc, this is an intentional change for 1.15.x (lemme look that up). Edit: also potentially related > jeremyjh/dialyxir#478 |
@benwilson512, I "fixed" it by doing: def application do
[extra_applications: [:crypto, :logger | extra_extra_applications(Mix.env())]]
end
def extra_extra_applications(:test), do: [:syntax_tools]
def extra_extra_applications(_), do: [] but this seems kinda hackish. I don't know what is including |
A slightly less hackish (at least consistent with 1.15 release notes change) would be: def project do
[
...
prune_code_paths: prune_code_paths(Mix.env())
]
end
defp prune_code_paths(:test), do: false
defp prune_code_paths(_), do: true |
As per https://github.com/paulo-ferraz-oliveira/absinthe/actions/runs/5426289985/jobs/9868142420, from commit https://github.com/paulo-ferraz-oliveira/absinthe/commit/b6c39deef09d93103e84f4de4bb4b7b9cf6235ce, you can see that CI passes (your branch without further changes). If you find the test-specific option I'll open a pull request for that, and we can continue there. |
Thanks for all of the work following up @paulo-ferraz-oliveira! |
This:
dialyxir
to CI (cached PLT)I may add comments to my own changes to clarify intention or source, if applicable (though the individual commits should tell part of the story already - they will have comments).