DM-12640: Implement Initial Butler Registry Hierarchy #14
Conversation
pschella
force-pushed
the
tickets/DM-12640
branch
3 times, most recently
from
1628416
to
8c7e189
Compare
pschella
changed the title
pschella
force-pushed
the
tickets/DM-12640
branch
6 times, most recently
from
2aaae47
to
cbeac36
Compare
pschella
changed the title
| if not isinstance(config, RegistryConfig): | ||
| if isinstance(config, str): | ||
| config = Config(config) | ||
| if isinstance(config, Config): |
There was a problem hiding this comment.
No, because it needs to build a RegistryConfig.
| @@ -37,18 +37,25 @@ class Registry(metaclass=ABCMeta): | |||
| """ | |||
| @staticmethod | |||
| def fromConfig(config): | |||
| cls = doImport(config['registry.cls']) | |||
| config = RegistryConfig(config['registry']) | ||
| else: | ||
| raise ValueError("Incompatible Registry configuration: {}".format(config)) | ||
| cls = doImport(config['cls']) | ||
| return cls(config=config) | ||
|
|
||
| def __init__(self, config): | ||
| """Constructor |
There was a problem hiding this comment.
docstring for constructor goes in class docstring.
https://developer.lsst.io/docs/py_docs.html#placement-of-class-docstrings
I won't mention this again in the review.
tests/test_schema.py
Outdated
| # | ||
| # LSST Data Management System | ||
| # | ||
| # Copyright 2008-2018 AURA/LSST. |
There was a problem hiding this comment.
Please use the updated preamble. Remove this copyright because it's in the COPYRIGHT file now.
| self.schemaFile = os.path.join(self.testDir, "../config/registry/default_schema.yaml") | ||
| self.config = SchemaConfig(self.schemaFile) | ||
| self.schema = Schema(self.config) | ||
| self.engine = create_engine('sqlite:///:memory:') |
There was a problem hiding this comment.
This is good. Means that the tests can all run in parallel.
| @@ -21,351 +21,148 @@ | |||
| # see <https://www.lsstcorp.org/LegalNotices/>. | |||
There was a problem hiding this comment.
I'm not sure how this file still has the old preamble. I fixed it in 0346dbc
| metadata = None # Needed to make disabled test_hsc not fail on import | ||
|
|
||
|
|
||
| def iterable(a): |
There was a problem hiding this comment.
I think this should go in the utils.py package.
| - a string -> return single element iterable (e.g. [a]) | ||
| - not iterable -> return single elment iterable (e.g. [a]). | ||
| """ | ||
| if hasattr(a, 'encode'): |
There was a problem hiding this comment.
Why can't we just see if it's a str?
There was a problem hiding this comment.
I had a debate about this with @natelust. This should be faster.
There was a problem hiding this comment.
In [3]: x = "xxx"
In [4]: %timeit hasattr(x, "encode")
109 ns ± 0.692 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
In [5]: %timeit isinstance(x, str)
102 ns ± 0.961 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
It doesn't look obviously different. I think we should go for the obvious implementation rather than looking for a side effect (and I can easily break yours by having a class with an encode method).
| Description of the column to be created. | ||
| Should always contain: | ||
| - name, descriptive name | ||
| - type, valid column type |
There was a problem hiding this comment.
This type is a type understood by the underlying database technology (in this case sqlaclhemy?) and not some abstract type that will be translated to something db-specific later on?
There was a problem hiding this comment.
Not quite. It should be a type in our type naming convention (which we haven't formalized yet).
|
|
||
| Raises | ||
| ------ | ||
| e : `ValueError |
There was a problem hiding this comment.
The docstring style guide does not include the e : for exceptions.
There was a problem hiding this comment.
Doesn't the a : T syntax require a name?
There was a problem hiding this comment.
pschella
force-pushed
the
tickets/DM-12640
branch
from
cbeac36
to
71f6590
Compare
pschella
force-pushed
the
tickets/DM-12640
branch
from
5f7a9fd
to
221a7fa
Compare
No description provided.