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

feat: Eliminate usage of OpenStruct #42

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

casperisfine
Copy link

OpenStruct design cause it to have non-local performance impact, especially with JIT compilers.

See ruby/ostruct#51 for full context, and it Ruby 3.3 OpenStruct usage raise a performance warnings:

gems/state_machines-audit_trail-2.0.2/lib/state_machines/audit_trail/transition_auditing.rb:38:
warning: OpenStruct use is discouraged for performance reasons

In this case I don't really see any advantage of OpenStruct over a simple PORO.

cc @seuros

@casperisfine casperisfine force-pushed the eliminate-open-struct branch from 72a5e26 to c021666 Compare January 22, 2024 15:37
@seuros
Copy link
Member

seuros commented Jan 22, 2024

I will add testing in ruby 3.3 in another PR

@casperisfine casperisfine force-pushed the eliminate-open-struct branch from c021666 to 0478ced Compare January 22, 2024 15:49
OpenStruct design cause it to have non-local performance impact,
especially with JIT compilers.

See ruby/ostruct#51 for full context,
and it Ruby 3.3 OpenStruct usage raise a performance warnings:

```
gems/state_machines-audit_trail-2.0.2/lib/state_machines/audit_trail/transition_auditing.rb:38:
warning: OpenStruct use is discouraged for performance reasons
```

In this case I don't really see any advantage of OpenStruct
over a simple PORO.
@casperisfine casperisfine force-pushed the eliminate-open-struct branch from 0478ced to 7cac29f Compare January 22, 2024 15:55
@casperisfine
Copy link
Author

Alright, after adding a few more empty methods to comply to the Transition interface, it's now passing.

I believe that strictly speaking this is backward compatible, since the initial transition accepting any possible method was AFAIK not documented. However it's not impossible some users may need to fix their code if they were wrongly calling methods outside of the interface.

If we want to preserve backward compatibility for these projects, we can also define a method_missing that just returns nil.

@seuros seuros changed the title Eliminate usage of OpenStruct fix: Eliminate usage of OpenStruct Jan 22, 2024
@seuros
Copy link
Member

seuros commented Jan 22, 2024

I will add the release workflow and merge this PR.

@casperisfine
Copy link
Author

Thank you very much 🙇

@seuros seuros changed the title fix: Eliminate usage of OpenStruct feat: Eliminate usage of OpenStruct Jan 22, 2024
@seuros seuros merged commit 4277c8b into state-machines:master Jan 22, 2024
15 checks passed
@seuros
Copy link
Member

seuros commented Jan 22, 2024

released

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.

3 participants