chore: upgrade stream_data by milmazz · Pull Request #149 · elixir-toniq/norm

milmazz · 2024-05-29T18:18:57Z

This PR also include the following changes:

Update credo config to the latest format.
Removes compilation warnings
Updates the CI workflow to use more recent otp/elixir versions (the erlef/setup-beam action can't find the original otp 23.3)
Removes unused config/config.exs file
Upgrades all the outdated dependencies
Fixes all the credo findings after the upgrade
Fixes some failing unit tests after the upgrade while trying to keep compatibility with previous versions

milmazz · 2024-05-29T18:20:01Z

lib/norm/core/selection.ex

  defp validate_selectors!([{_key, inner} | rest]), do: validate_selectors!(inner) and validate_selectors!(rest)
  defp validate_selectors!([_key | rest]), do: validate_selectors!(rest)
-  defp validate_selectors!(other), do: raise ArgumentError, "select expects a list of keys but received: #{inspect other}"
+  defp validate_selectors!(other), do: raise(ArgumentError, "select expects a list of keys but received: #{inspect other}")


Fixes the following compilation warning:

milmazz · 2024-05-29T18:20:26Z

config/config.exs

@@ -1,30 +0,0 @@
-# This file is responsible for configuring your application
-# and its dependencies with the aid of the Mix.Config module.
-use Mix.Config


Remove unused config file

milmazz · 2024-05-29T19:44:06Z

mix.exs

      app: :norm,
      version: @version,
-      elixir: "~> 1.7",
+      elixir: "~> 1.11",


The GH workflow file seems to be testing this matrix: elixir: [1.11], otp: [23.3]

I guess that matrix should be updated, but I prefer to ask first here first.

milmazz · 2024-05-29T19:46:01Z

test/norm/core/schema_test.exs

      assert {:error, errors} = conform(%User{name: :foo, age: "31", email: 42}, User.s())

-      assert errors == [
+      assert MapSet.equal?(MapSet.new(errors), MapSet.new([


Unit test failing locally, at least with 1.17.0-rc.0-otp-27

milmazz · 2024-05-29T19:46:44Z

mix.exs

  use Mix.Project

  @version "0.13.0"
+  @source_url "https://github.com/elixir-toniq/norm"


Avoid duplicating this value, updated source url.

milmazz · 2024-05-29T19:47:40Z

mix.exs

    [
      {:credo, "~> 1.4", only: [:dev, :test], runtime: false},
-      {:stream_data, "~> 0.5", optional: true},
+      {:stream_data, "~> 0.6 or ~> 1.0", optional: true},


Main reason for this PR, updating stream_data that's causing some upgrade conflicts on my end.

milmazz · 2024-05-29T21:38:26Z

.github/workflows/elixir.yml

-        elixir: [1.11]
-        otp: [23.3]
+        elixir: [1.16.1]
+        otp: [26.2]


23.3 not found

mhanberg · 2024-05-30T15:30:14Z

Can you bump the elixir and Erlang versions in the gha to OTP 25 and whatever the lowest version of elixir is that supports OTP 25

milmazz · 2024-05-30T15:44:00Z

@mhanberg hey 👋 , thanks for taking a look at this. I already applied your suggestion.

mhanberg · 2024-05-30T15:45:35Z

Hmm, I'm not sure why the actions runs are not respecting that. I'll take a look once I get back to my computer

milmazz · 2024-05-30T15:52:30Z

I see the results from the Analysis and Tests phase taking the right versions now. But let me look why credo is failing so abruptly and fix some failing unit tests.

I will try to install the otp/elixir combination you suggested locally, probably I will not be able to do it because those are a bit old and I'm currently using an Apple M2, but let me try, I will report back.

mhanberg · 2024-05-30T15:58:31Z

oh, i think my phone was just showing the required checks, so the new ones didn't show up

milmazz · 2024-05-30T16:05:04Z

test/norm/contract_test.exs

+    expected = if version().minor >= 13,
+      do: ~r/got: `@contract foo\(n\)`/, else: ~r/got: `@contract\(foo\(n\)\)`/


I found this pattern while inspecting test/norm/core/selection_test.exs, so I decided to follow that approach to keep compatibility with previous versions.

milmazz · 2024-05-30T16:06:33Z

mix.lock

@@ -1,14 +1,14 @@
 %{
-  "bunt": {:hex, :bunt, "0.2.0", "951c6e801e8b1d2cbe58ebbd3e616a869061ddadcc4863d0a2182541acae9a38", [:mix], [], "hexpm", "7af5c7e09fe1d40f76c8e4f9dd2be7cebd83909f31fee7cd0e9eadc567da8353"},


After running mix hex.outdated, I noticed that everything was outdated, so, I decided to bump all the dependencies, most of them for development purposes (ex_doc and credo)

milmazz · 2024-05-30T16:24:49Z

@mhanberg re: credo failures

UPDATE: Credo it's fine up to Elixir 1.16, I was initially locally using v1.17-rc.0 and there is a change is in the elixir tokenizer. ~~I will send a fix to credo if I can't find a related PR~~. Someone is already taking care of that issue: rrrene/credo#1136

This is a little bit wtf, but here what's happening. If you run mix credo you mostly get these failures:

11:19:48.165 [error] Task #PID<0.254.0> started from #PID<0.216.0> terminating
** (CaseClauseError) no case clause matching: {:ok, 63, 1, [], [{:eol, {62, 4, 1}}, {:end, {62, 1, nil}}, {:eol, {61, 6, 1}}, {:end, {61, 3, nil}}, {:eol, {60, 8, 1}}, {:end, {60, 5, nil}}, {:eol, {59, 63, 1}}, {:")", {59, 62, nil}}, {:"]", {59, 61, nil}}, {:bin_string, {59, 58, nil}, [">"]}, {:",", {59, 56, 0}}, {:")", {59, 55, nil}}, {:identifier, {59, 51, ~c"opts"}, :opts}, {:",", {59, 49, 0}}, {:identifier, {59, 44, ~c"specs"}, :specs}, {:., {59, 43, nil}}, {:identifier, {59, 38, ~c"union"}, :union}, {:"(", {59, 37, nil}}, {:paren_identifier, {59, 31, ~c"to_doc"}, :to_doc}, {:",", {59, 29, 0}}, {:bin_string, {59, 15, nil}, ["#Norm.OneOf<"]}, {:"[", {59, 14, nil}}, {:"(", {59, 13, nil}}, {:paren_identifier, {59, 7, ~c"concat"}, :concat}, {:eol, {58, 32, 1}}, {:do, {58, 30, nil}}, {:")", {58, 28, nil}}, {:identifier, {58, 24, ~c"opts"}, :opts}, {:",", {58, 22, 0}}, {:identifier, {58, 17, ~c"union"}, :union}, {:"(", {58, 16, nil}}, {:paren_identifier, {58, 9, ~c"inspect"}, :inspect}, {:identifier, {58, 5, ~c"def"}, :def}, {:eol, {56, 27, 2}}, {:alias, {56, 20, ~c"Algebra"}, :Algebra}, {:., {56, 19, nil}}, {:alias, {56, 12, ~c"Inspect"}, :Inspect}, {:identifier, {56, 5, ~c"import"}, :import}, {:eol, {55, 21, 1}}, {:do, {55, 19, nil}}, {:alias, {55, 11, ...}, :Inspect}, {:identifier, {55, ...}, :defimpl}, {:eol, {...}}, {:end, ...}, {...}, ...], []}
    (credo 1.7.6) lib/credo/code.ex:138: Credo.Code.to_tokens/2
    (credo 1.7.6) lib/credo/check/readability/large_numbers.ex:43: Credo.Check.Readability.LargeNumbers.run/2
    (credo 1.7.6) lib/credo/check/readability/large_numbers.ex:2: Credo.Check.Readability.LargeNumbers.do_run_on_source_file/3
    (elixir 1.17.0-rc.0) lib/task/supervised.ex:101: Task.Supervised.invoke_mfa/2
    (elixir 1.17.0-rc.0) lib/task/supervised.ex:36: Task.Supervised.reply/4
Function: &:erlang.apply/2
    Args: [#Function<2.75132833/1 in Credo.Check.Readability.LargeNumbers.do_run_on_all_source_files/3>, [%SourceFile<lib/norm/core/any_of.ex>]]

So, I went to the credo code, and certainly noticed the mismatch here:

 def to_tokens(source, filename \\ "nofilename") when is_binary(source) do
    source
    |> String.to_charlist()
    |> :elixir_tokenizer.tokenize(1, file: filename)
    |> case do
      # Elixir < 1.6
      {_, _, _, tokens} ->
        tokens

      # Elixir >= 1.6
      {:ok, tokens} ->
        tokens

      # Elixir >= 1.13
      {:ok, _, _, _, tokens} ->
        tokens

      {:error, _, _, _, tokens} ->
        tokens
    end
  end

So, I did modified that file with this diff:

# diff lib/credo/code.ex.bak lib/credo/code.ex
148c148
<       {:ok, _, _, _, tokens} ->
---
>       {:ok, _, _, _, tokens, _} ->

Run mix deps.compile --force credo and it worked hahaha

So, let me fix those issues first and then see if credo introduced a bug at some point or whatever.

milmazz · 2024-05-30T16:59:35Z

Ok, now everything passes locally:

mhanberg · 2024-05-30T18:27:43Z

.credo.exs

+          {Credo.Check.Consistency.ExceptionNames, []},
+          {Credo.Check.Consistency.LineEndings, []},
+          {Credo.Check.Consistency.ParameterPatternMatching, []},
+          {Credo.Check.Consistency.SpaceAroundOperators, []},


I think you actually altered the configured checks when you updated the config, do you mind ensuring the same checks enabled as before?

Updated 👍

mhanberg · 2024-06-01T18:26:00Z

Sorry, one last ask. Can you revert any unrelated style changes or credo changes that were due to the checks being changed?

antedeguemon · 2025-01-09T22:47:01Z

Hey @milmazz! 🖖

Sorry for the ping, but is there any update on this?

If you need (and allow me to), I can open a separate pull request cherry-picking your fixes for the compilation warnings.
This would fix the warnings from compiling other libraries that depend on norm.

milmazz · 2025-01-10T20:26:34Z

@antedeguemon Thanks for the ping! I updated this branch and now we should be ready for another review from @mhanberg

mhanberg · 2025-01-10T20:34:31Z

Looks like just needs some credo fixes and I think it's good to go

milmazz · 2025-01-10T21:23:47Z

Those are the credo changes you asked to remove in your previous comment @mhanberg

mhanberg · 2025-01-10T21:28:38Z

Those are the credo changes you asked to remove in your previous comment @mhanberg

What i was previously referring to was changing the credo config file so that it added/removed checks, requiring other code changes.

The ones that are currently failing are from updating Credo, as the MapJoin check was added after the previous version of Credo we were on.

These ones are new default credo checks and i'm okay with fixing those.

milmazz · 2025-01-10T21:28:54Z

@mhanberg To move on with this discussion, I put it back the refactoring opportunities found by credo.

chore: upgrade stream_data

bd8e5f6

milmazz commented May 29, 2024

View reviewed changes

milmazz added 2 commits May 29, 2024 14:39

reduce duplication on mix.exs

8b9034c

fix order error in unit test

651f0ac

milmazz commented May 29, 2024

View reviewed changes

bump otp & elixir versions in ci

abea59a

milmazz commented May 29, 2024

View reviewed changes

.github/workflows/elixir.yml Outdated

elixir: [1.11]

otp: [23.3]

elixir: [1.16.1]

otp: [26.2]

Copy link

Contributor Author

milmazz May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

23.3 not found

milmazz added 2 commits May 30, 2024 10:24

minor updates in GH workflow

6baaf02

revert weird change on mix.exs

71c0fa5

Follow @mhanberg suggestion to combine otp 25 with elixir 1.13

9a50feb

upgrade all the dependencies

f33dead

failing tests should pass now

698c913

milmazz commented May 30, 2024

View reviewed changes

mhanberg reviewed May 30, 2024

View reviewed changes

milmazz force-pushed the m/chore/upgrade-stream-data branch from 9de8549 to 698c913 Compare January 10, 2025 19:42

update deps once again

6679e38

put back refactoring suggested by credo

8b20e8b

mhanberg merged commit 0ef51be into elixir-toniq:master Jan 10, 2025

milmazz deleted the m/chore/upgrade-stream-data branch January 11, 2025 00:33

		expected = if version().minor >= 13,
		do: ~r/got: `@contract foo\(n\)`/, else: ~r/got: `@contract\(foo\(n\)\)`/

		@@ -1,14 +1,14 @@
		%{
		"bunt": {:hex, :bunt, "0.2.0", "951c6e801e8b1d2cbe58ebbd3e616a869061ddadcc4863d0a2182541acae9a38", [:mix], [], "hexpm", "7af5c7e09fe1d40f76c8e4f9dd2be7cebd83909f31fee7cd0e9eadc567da8353"},

Conversation

milmazz commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhanberg commented May 30, 2024

Uh oh!

milmazz commented May 30, 2024

Uh oh!

mhanberg commented May 30, 2024

Uh oh!

milmazz commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhanberg commented May 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milmazz commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milmazz commented May 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhanberg commented Jun 1, 2024

Uh oh!

antedeguemon commented Jan 9, 2025

Uh oh!

milmazz commented Jan 10, 2025

Uh oh!

mhanberg commented Jan 10, 2025

Uh oh!

milmazz commented Jan 10, 2025

Uh oh!

mhanberg commented Jan 10, 2025

Uh oh!

milmazz commented Jan 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

milmazz commented May 29, 2024 •

edited

Loading

milmazz commented May 30, 2024 •

edited

Loading

milmazz commented May 30, 2024 •

edited

Loading