-
Notifications
You must be signed in to change notification settings - Fork 1
Improve module structure #140
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
Conversation
b6ff40d to
822a173
Compare
7722937 to
3a792c5
Compare
3a792c5 to
ecb1889
Compare
olliesilvester
left a comment
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.
Thanks, the structure is nicer here. We just need to make sure we're consistent with making private classes/modules and exposing the right bits in the __init__.pys. Ophyd-async is a good example of how it should look, eg see https://github.com/bluesky/ophyd-async/blob/main/src/ophyd_async/epics/adcore/__init__.py
src/daq_config_server/models/converters/beamline_parameters/_converters.py
Show resolved
Hide resolved
5c26426 to
b929a6a
Compare
b929a6a to
ec2409c
Compare
47eaf30 to
cc0f1c4
Compare
olliesilvester
left a comment
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.
Looks much neater thanks, just a couple of things, but feel free to disagree
|
|
||
| __all__ = [ | ||
| "GenericLookupTable", | ||
| "parse_lut", |
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.
Might have missed something but I don't think we need to exposeparse_lut - I think we only use it in the tests right? I think tests should be able to import from private modules, we can probably add a typing rule so that files in the test folder can do this
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.
That's a good point, I'll remove it
| from pydantic import BaseModel | ||
|
|
||
|
|
||
| class ConfigModel(BaseModel): ... |
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.
Not relevant to this PR really but can we add a comment just to say it should be the parent class to all other config models
olliesilvester
left a comment
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.
Thanks, looks neat
Fixes #134
Splits converters and models into their own modules based on the type of config file they deal with. Also adds a public API so that all models can be imported with
from daq_config_server.models import SomeModel