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

Optional Model IDs #17

Merged
merged 7 commits into from
Apr 24, 2022
Merged

Optional Model IDs #17

merged 7 commits into from
Apr 24, 2022

Conversation

ztnel
Copy link
Owner

@ztnel ztnel commented Apr 24, 2022

Description

Some models do not require a unique identifier (id property) from the perspective of a producer, however it is used internally to determine object equality. For example, it would make sense that a telemetry document from a sensor may simply not require a unique identifier:

{
    "temp": 45.0,
    "timestamp": 1650827424.46608
}

Originally I had enforced that the producer must generate a unique identifier before committing the model to the system state. However, this breaks encapsulation since the id is only used internally by myosin. I have changed the model parameters to optionally allow the producer to set a unique identifier which is useful for REST API models with database transactions. If unset the BaseModel will generate a UUID which any module can access through the same id property.

Resolutions

  • Implement automatic id generation for models without a uuid requirement
  • fix coverage ignores globbing
  • Remove type alias export for primary keys

@ztnel ztnel added enhancement New feature or request core core feature implementation labels Apr 24, 2022
@ztnel ztnel self-assigned this Apr 24, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2022

Codecov Report

Merging #17 (ed20f3a) into master (576250f) will increase coverage by 1.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #17      +/-   ##
==========================================
+ Coverage   94.09%   95.09%   +1.00%     
==========================================
  Files          12        8       -4     
  Lines         220      204      -16     
  Branches       27       28       +1     
==========================================
- Hits          207      194      -13     
+ Misses         11        8       -3     
  Partials        2        2              
Impacted Files Coverage Δ
myosin/models/base.py 96.96% <100.00%> (+0.30%) ⬆️
myosin/models/state.py 100.00% <100.00%> (ø)
myosin/__init__.py
myosin/__version__.py
myosin/typing/__init__.py
myosin/state/__init__.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 576250f...ed20f3a. Read the comment docs.

@ztnel ztnel merged commit 18d228e into master Apr 24, 2022
@ztnel ztnel deleted the idset branch April 24, 2022 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core core feature implementation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants