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

Inheritance (sub class from Unit) no longer works #339

Closed
kreintjes opened this issue Jan 30, 2024 · 4 comments · Fixed by #340
Closed

Inheritance (sub class from Unit) no longer works #339

kreintjes opened this issue Jan 30, 2024 · 4 comments · Fixed by #340
Assignees

Comments

@kreintjes
Copy link
Contributor

In our app we create a sub class from Unit to apply some custom behaviour (#240 (comment)). This worked fine Ruby-Units 3.0 and before, but stopped working since Ruby-Units 4.0. If we try this now every .new call crashes with a undefined method keys' for nil:NilClass (NoMethodError)error inunit.rb:363:in unit_regex. This essentially breaks the functionality introduced in #241.

To reproduce:

Add Ruby-Units 4.0.x to the Gemfile, and execute the following:

class ChildUnit < Unit; end
ChildUnit.new('1 l')

We are suspecting this has something to do with the refactor in #274. Any suggestions on how to fix this would be welcome!

@olbrich
Copy link
Owner

olbrich commented Jan 30, 2024

@kreintjes Confirmed, I'll look into this soon.

@olbrich
Copy link
Owner

olbrich commented Jan 31, 2024

@kreintjes Please take a look at the associated PR and give it a shot. Let me know if it fixes your issues.

Out of curiosity, what is motivating you to create a subclass?

@kreintjes
Copy link
Contributor Author

@olbrich I tried it both in my sample project as well as in our real project and it both works great! Thanks for the quick fix!

We use a sub class of RubyUnits to override the eliminate_terms method with a simpler variant that does less simplification of the units after applying operations. E.g. when having Unit.new('10 l') / Unit.new('5 l') we leave this as 2 l/l instead of 2. See #240 for details.

@kreintjes
Copy link
Contributor Author

Nice you released this so quickly @olbrich! ❤️

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

Successfully merging a pull request may close this issue.

2 participants