CargoSense / vex

Data Validation for Elixir
MIT License
595 stars 60 forks source link

Fix warnings on newer Elixir versions #74

Closed IvanIvanoff closed 8 months ago

IvanIvanoff commented 8 months ago

Running with newer Elixir versions, the compiling Vex leads to some warnings. These warnings are fixed in a backwards compatible way.

The fixes include:

Example: When compiling/running tests, I get a lot of warnings about this:

image
jfornoff commented 8 months ago

Do not use min..max in a pattern match without explicitly matching the step.

I don't see where the step of the Range is incorporated, isn't it still ignored by the validation?

IvanIvanoff commented 8 months ago

The stepless range validation is introduced here. It will only emit a warning and won't break until Elixir 2.0, though. Here's a short example using the Elixir main branch:

➜  ~ iex
Erlang/OTP 26 [erts-14.2.1] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [jit]

Interactive Elixir (1.17.0-dev) - press Ctrl+C to exit (type h() ENTER for help)
iex(1)> defmodule T do
...(1)> def foo(min..max), do: {min, max}
...(1)> end
warning: min..max inside match is deprecated, you must always match on the step: min..max//var or min..max//_ if you want to ignore it
  iex:2: T.foo/1

I think more people will be willing to work locally with the main branch so they can test all the things related to the type system development that are being pushed now.

jfornoff commented 8 months ago

Ah yes, I was referring to the Vex functionality. Using min..max//_ seems like a good choice here, since the Vex range validation indeed doesn't take the step into account. Maybe also open an issue about that? Seems easily fixable, but we don't have to block this PR on that.

IvanIvanoff commented 8 months ago

I was referring to the Vex functionality

Ah, yes, I understood this as what checks does the Elixir compile make.

Using min..max//_ seems like a good choice here

The mix.exs file specifies elixir: "~> 1.6". Using min..max//_ will bump that requirement to 1.12 and I didn't know if that is ok. Pattern matching on the %Range{} struct on the other hand does not require bumping the Elixir version. (The same motivation is put in the commit message as well)

jfornoff commented 8 months ago

Makes sense, thanks for elaborating! :-)