norm icon indicating copy to clipboard operation
norm copied to clipboard

chore: upgrade stream_data

Open milmazz opened this issue 1 year ago • 8 comments

This PR also include the following changes:

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

milmazz avatar May 29 '24 18:05 milmazz

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

mhanberg avatar May 30 '24 15:05 mhanberg

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

milmazz avatar May 30 '24 15:05 milmazz

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

mhanberg avatar May 30 '24 15:05 mhanberg

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.

milmazz avatar May 30 '24 15:05 milmazz

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

mhanberg avatar May 30 '24 15:05 mhanberg

@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: https://github.com/rrrene/credo/pull/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

Screenshot 2024-05-30 at 11 24 01 AM

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

milmazz avatar May 30 '24 16:05 milmazz

Ok, now everything passes locally:

Screenshot 2024-05-30 at 11 59 01 AM

milmazz avatar May 30 '24 16:05 milmazz

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

mhanberg avatar Jun 01 '24 18:06 mhanberg

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.

antedeguemon avatar Jan 09 '25 22:01 antedeguemon

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

milmazz avatar Jan 10 '25 20:01 milmazz

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

mhanberg avatar Jan 10 '25 20:01 mhanberg

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

milmazz avatar Jan 10 '25 21:01 milmazz

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.

mhanberg avatar Jan 10 '25 21:01 mhanberg

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

milmazz avatar Jan 10 '25 21:01 milmazz