folz / bento

:bento: A fast, correct, pure-Elixir library for reading and writing Bencoded metainfo (.torrent) files.
Mozilla Public License 2.0
95 stars 14 forks source link

Fix some type-related errors #13

Closed flisk closed 1 year ago

flisk commented 3 years ago

This fixes a couple of typespecs and type references that didn't seem right.

folz commented 3 years ago

Hi @Flisk, I'm just now seeing this PR. Thanks for submitting these changes! iirc these types worked for me, but that was a while back and maybe spec has changed, or my implementation was wrong and I didn't notice. What were your steps for testing the new typespecs so I can do the same?

flisk commented 2 years ago

Hey, sorry for taking so long to get back. For some reason I didn't get notified of your response.

I ran all of my tests under Elixir 1.13.2 with Erlang 24.2.

I described 38b6373 a little further in the extended commit message. To test my changes, I ran mix dialyzer from a separate dummy project that depends on bento.

A simple call to Bento.encode!/1 is enough to trip up dialyzer:

%{ test: "Hello, World!" } |> Bento.encode!()
The function call will not succeed.

Bento.encode!(%{:test => <<72, 101, 108, 108, 111, 44, 32, 87, 111, 114, 108, 100, 33>>})

will never return since the 1st arguments differ
from the success typing arguments:

(
  binary()
  | maybe_improper_list(
      binary() | maybe_improper_list(any(), binary() | []) | byte(),
      binary() | []
    )
)

The definition of Bento.Encoder.encode/1 suggests to me that Bento.Encoder.t is meant to be the result type of encoding operations, not the input.

a381d31 can be tested by running mix dialyzer on bento itself. It fixes the following warnings:

lib/bento.ex:49: Unknown type 'Elixir.Stream':t/0
lib/bento/encoder.ex:49: Unknown type 'Elixir.Stream':t/0
lib/bento/metainfo.ex:14: Unknown type 'Elixir.MultiFile':t/0
lib/bento/metainfo.ex:14: Unknown type 'Elixir.SingleFile':t/0

Changing Stream.t to Enumerable.t was a guess for the most part. I couldn't find a Stream.t type in any version of Elixir from 1.4 and up, and Elixir's own typespecs for functions in the Stream module also use Enumerable.t.

I've attached another fix for these two warnings which occur on mix dialyzer:

lib/bento.ex:74: Unknown function 'Elixir.Poison.Decode':decode/2
lib/bento.ex:86: Unknown function 'Elixir.Poison.Decode':decode/2
mogeko commented 1 year ago

For all guys who encounter this problem.

It seems that you can alleviate this problem by calling Bento.Encoder.encode/1 directly.

%{ test: "Hello, World!" } |> Bento.Encoder.encode() |> IO.iodata_to_binary()

But this is just a temporary solution.

flisk commented 1 year ago

Sooo, how 'bout that patch? It's almost been a year 😅

folz commented 1 year ago

Honest answer is I'm not actively working on this project anymore and it has long since slipped my mind. If this is something you still use, would you be interested in becoming a contributor? Lmk and I can add you with commit rights so you can merge this and land other changes as needed.

mogeko commented 1 year ago

Honest answer is I'm not actively working on this project anymore and it has long since slipped my mind. If this is something you still use, would you be interested in becoming a contributor? Lmk and I can add you with commit rights so you can merge this and land other changes as needed.

I'm sorry to hear that, and I really appreciate your contributions.

I have a project that has this library as a core dependency. I'm very happy to be a contributor to this library, and I'd love to collaborate with @Flisk if he wants to.

folz commented 1 year ago

Appreciate it - it was a fun thing to build and I'm glad you're still finding use for it! I'm happy to add you as a contributor too, and move the hex package to an organization. I'll try to do this within a week, and feel free to ping me again if it seems like I've let it slip.

folz commented 1 year ago

Hey @mogeko - I've sent you an invite to collaborate on this repository. Could you also let me know your username on hex.pm?

mogeko commented 1 year ago

Thank u! My username at hex.pm is mogeko, same as in GitHub.

folz commented 1 year ago

Great. I moved the bento package to a new public organization, also called bento, and added you to it. You should be able to make new releases now.