-
Notifications
You must be signed in to change notification settings - Fork 0
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
Structure changes and Config support #12
Conversation
6fe93fe
to
599871d
Compare
module Unitsdb | ||
class Dimensions | ||
class Quantity < Lutaml::Model::Serializable | ||
model Config.model_for(:dimension_quantity) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@suleman-uzair what's the reason for using model
instead of using lutaml-model to implement the "thing" directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ronaldtse, We needed to extend some classes for the ease of internal processing in unitsml-ruby (eg, https://github.com/unitsml/unitsml-ruby/pull/27/files#diff-fa369bed40e5c1361cef8135c292ef660ff67ba05abbdd45b6111b860eb735c6 ), model
allows us to configure the classes available in unitsml-ruby while keeping the unitsdb-ruby standalone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not possible by inheritance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we do not want usage of "model" because lutaml-model should provide better ways to natively support such re-use. In any case this PR is fine. But I do want to see how we can replace the model ...
line with something more native.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not possible by inheritance?
@ronaldtse, We can, but that would require us to update the attribute
using that (parent class) as a datatype with our/extended class.
class A < Lutaml::Model::Serializable; end
class B < Lutaml::Model::Serializable
attribute :a, A
# ... other attributes
xml do
map_attribute :a, to: :a
end
end
class CustomClass < A
attribtue :other_attr, other_attr
xml do
attribute :OtherAttr, to: :other_attr
end
end
In the above example, the CustomClass
won’t be used automatically, so we will have to update all the classes using A
class as a datatype.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we do not want usage of "model" because lutaml-model should provide better ways to natively support such re-use. In any case this PR is fine. But I do want to see how we can replace the model ... line with something more native.
@ronaldtse Please let me know what you have in mind to tackle this specific scenario in the lutaml-model?
can you share any examples in terms of code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@suleman-uzair the use case seems related to this, where the difference is that you would like to:
- Replace the attribute
a
's value type withCustomClass
, and - Keep the mapping of
map_attribute "a"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ronaldtse, This may be helpful for cases where an attribute is not widely used; however, using a new class for a widely used attribute can complicate it, making it more difficult to understand and maintain.
For instance, a mrow
tag is permitted in nearly all tags in MathML. Therefore, if we were to update the mrow
class, we would need to update or reopen almost all the classes of plurimath/mml for this.
# new class for Mrow
class NewMrow < Mrow; end
# using NewMrow class in Mml
class Mml::MathWithNamespace < Lutaml::Model::Serializable
attribute :mrow, NewMrow, collection: true
end
class Mml::MathWithNilNamespace < Lutaml::Model::Serializable
attribute :mrow, NewMrow, collection: true
end
class Mml::Mfenced < Lutaml::Model::Serializable
attribute :mrow, NewMrow, collection: true
end
# the rest of the classes.
Let me know if I’m missing something here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@suleman-uzair I understand. Can you please help generalize this problem and create an issue in https://github.com/lutaml/lutaml-model so we can make this use case available for users of lutaml-model?
@ronaldtse, Should I merge this? |
@suleman-uzair yes please help merge and release. Thanks! |
dependent-gems workflow
2d98fc5
to
0c8ddbd
Compare
This PR updates the file/class structure and adds a
config
class allowing users to customize content in their classes.closes #3