Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Promote use of keywords lists instead maps for job list #333

Open
eksperimental opened this issue Mar 22, 2021 · 3 comments
Open

Promote use of keywords lists instead maps for job list #333

eksperimental opened this issue Mar 22, 2021 · 3 comments

Comments

@eksperimental
Copy link
Contributor

Hi @PragTob, first of all, thank you for such an amazing library, it helps me every now and then to make Elixir an even better language.

I would like to suggest to change from maps to keywords lists for the list of jobs.
The reason is that since map is not an ordered structure, it may change the order of the execution depending on the name of the key.
As of now, Benchee.run/2 works with either maps and keyword lists since it takes any enumerable.
The specs for this function should also be updated.

The examples in the README file would also look like this.

Benchee.run(
  [
    "flat_map": fn -> Enum.flat_map(list, map_fun) end,
    "map.flatten": fn -> list |> Enum.map(map_fun) |> List.flatten() end
  ]
)

This is the way I set all my benchmarks.
Thank you.

@PragTob
Copy link
Member

PragTob commented Feb 19, 2022

👋

Hello, I believe I already gave you the "Hands, sorry" talk so I'm gonna skip it 😁

Specs absolutely should be updated. Iirc we even have tests to make sure this works. As for making it the default.... I'm not sure.

Order of execution shouldn't really matter and the results are sorted by performance anyhow. I think we could probably highlight better that it's an option but I don't see a huge reason here to make it the default.

I like maps because they immediately convey that the keys/names shall not be duplicated :)

@eksperimental
Copy link
Contributor Author

eksperimental commented Feb 19, 2022

Hi.
I can't remember the use case because this was almost a year ago, but order made a difference.
I think it was it was that I wanted to see some results before the whole suite was tested. Made some changes, see if it worked and if not stop it straight away.
But with maps the order is not guaranteed, so I had to move to keyword lists in order to achieve this.

@PragTob
Copy link
Member

PragTob commented Feb 19, 2022

Ah I see, the pre_check feature that runs every job once wouldn't help? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants