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

ft: add struct for ClientHandler state #369

Closed

Conversation

hauleth
Copy link
Contributor

@hauleth hauleth commented Jun 27, 2024

What kind of change does this PR introduce?

Adds struct for keeping ClientHandler state.

Additional context

This approach allows newer Elixir versions to optimise code better, like for
example avoiding tests for whether passed argument is a map (it has to, as it
was already checked in the function head). It also allows compiler to catch at
compile time that you have accessed or you try to update incorrect field
(because of typo).

@hauleth hauleth requested a review from a team as a code owner June 27, 2024 13:07
@hauleth hauleth marked this pull request as draft June 27, 2024 13:25
@hauleth hauleth force-pushed the ft/add-struct-fro-ClientHandler-state branch 2 times, most recently from 943a76a to 58b9b49 Compare June 28, 2024 14:00
@hauleth hauleth changed the base branch from main to v2 June 28, 2024 14:07
hauleth added 2 commits June 28, 2024 16:08
This is used to allow Elixir compiler to provide minuscule optimisation,
as with `%{} = data` it will no longer need to check if `data` is an
atom when doing `data.foo`. It also makes it harder to accidentally
access non-existing field as new Elixir versions can check accessed
fields names.
@hauleth hauleth force-pushed the ft/add-struct-fro-ClientHandler-state branch from 58b9b49 to dd7e376 Compare June 28, 2024 14:08
@abc3
Copy link
Member

abc3 commented Jun 28, 2024

Let's postpone this. It adds complexity with hot code upgrades, and I think the struct will add latency

@hauleth hauleth closed this Jul 2, 2024
@hauleth hauleth deleted the ft/add-struct-fro-ClientHandler-state branch July 8, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants