elixir-ecto / ecto_sql

SQL-based adapters for Ecto and database migrations
https://hexdocs.pm/ecto_sql
Apache License 2.0
571 stars 312 forks source link

lib/ecto/adapters/myxql/connection.ex: refactored improper lists #587

Closed bradhanks closed 9 months ago

bradhanks commented 9 months ago

refactored 10 lists

5 had a string literal as the tail 1 was a variable with a is_binary guard 4 were Integer.to_string functions

greg-rychlewski commented 9 months ago

Hi @bradhanks ,

Improper lists are 100% intentional. They save some bytes when sending the query string to the database.

bradhanks commented 9 months ago

Hi @bradhanks ,

Improper lists are 100% intentional. They save some bytes when sending the query string to the database.

oh nice. learned something new :)

greg-rychlewski commented 9 months ago

Sorry I mixed up the adapter behaviour with the driver behaviour (Postgrex, etc.).

In this case it's not about sending it to the database but rather this construct will be immediately passed to IO.iodata_to_binary to create the query string. And iodata can work with improper lists. So basically there is no need to wrap the value in a list.

bradhanks commented 9 months ago

Sorry I mixed up the adapter behaviour with the driver behaviour (Postgrex, etc.).

In this case it's not about sending it to the database but rather this construct will be immediately passed to IO.iodata_to_binary to create the query string. And iodata can work with improper lists. So basically there is no need to wrap the value in a list.

Just to solidify my own understanding, if you don't mind, are there issues with the below statement?

There's really no issue using a cons operator to add a string to the end of an iolist. The list isn't improper if all the elements are binaries.

greg-rychlewski commented 9 months ago

It looks like there is a slight misunderstanding in it.

An improper list isn't about whether the elements are binaries but whether the last element's tail is the empty list. Every proper list can be represented as [head | tail] where tail is another list. So for example, [1, 2] is actually [1 | [2 | []]]. If you built a similar improper list it would instead be just [1 | 2] (the last element is not a list).

Where binaries play a special role is in iolists. This is the definition from erlang:

iolist ::= []
        |   Binary
        |   [iohead | iolist]
        ;
iohead ::= Binary
        |   Byte (integer in the range [0..255])
        |   iolist
        ;

So when creating an improper list inside of an iolist, the last tail can be a binary but not an integer.

bradhanks commented 9 months ago

It looks like there is a slight misunderstanding in it.

An improper list isn't about whether the elements are binaries but whether the last element's tail is the empty list. Every proper list can be represented as [head | tail] where tail is another list. So for example, [1, 2] is actually [1 | [2 | []]]. If you built a similar improper list it would instead be just [1 | 2] (the last element is not a list).

Where binaries play a special role is in iolists. This is the definition from erlang:

iolist ::= []
        |   Binary
        |   [iohead | iolist]
        ;
iohead ::= Binary
        |   Byte (integer in the range [0..255])
        |   iolist
        ;

So when creating an improper list inside of an iolist, the last tail can be a binary but not an integer.

It looks like there is a slight misunderstanding in it.

An improper list isn't about whether the elements are binaries but whether the last element's tail is the empty list. Every proper list can be represented as [head | tail] where tail is another list. So for example, [1, 2] is actually [1 | [2 | []]]. If you built a similar improper list it would instead be just [1 | 2] (the last element is not a list).

Where binaries play a special role is in iolists. This is the definition from erlang:

iolist ::= []
        |   Binary
        |   [iohead | iolist]
        ;
iohead ::= Binary
        |   Byte (integer in the range [0..255])
        |   iolist
        ;

So when creating an improper list inside of an iolist, the last tail can be a binary but not an integer.

ok so an iolist can be improper or proper depending on if it ends in an empty list or binary.

greg-rychlewski commented 9 months ago

Yeah that is correct.

bradhanks commented 9 months ago

Yeah that is correct. Thanks for taking the time to explain that to me! 👍