daf_persistence - Tickets/dm 11407 #74
Conversation
|
|
||
|
|
||
| class RepositoryCfgPosixFormatter(): | ||
| def write(butlerLocation, cfg): |
There was a problem hiding this comment.
read and write are very generic names and you do from .fmtPosixRepositoryCfg import * which has a small chance of messing up with other names. I'd probably add __all__ = [] before all imports to avoid these names being picked up by import *. Two other alternatives:
- replace
from .fmtPosixRepositoryCfg import *withfrom . import fmtPosixRepositoryCfg(I don't think you need any names from this module - rename
readandwriteinto "local" names_readand_write. Still__all__ = []is useful if you keepimport *because all non-underscored names are still being imported if you don't define__all__
| ---------- | ||
| cfg : RepositoryCfg instance | ||
| The RepositoryCfg to be serialized. | ||
| butlerLocation : ButlerLocation |
There was a problem hiding this comment.
The order should probably correspond to order in the argument list.
| loc = os.path.split(loc)[0] # remove the `repoistoryCfg.yaml` file name | ||
| if loc is None or cfg.root == loc: | ||
| cfg = copy.copy(cfg) | ||
| loc = cfg.root |
There was a problem hiding this comment.
Does this line do anything?
| return | ||
| except IOError as e: | ||
| if e.errno != errno.ENOENT: # ENOENT is 'No such file or directory' | ||
| raise e |
There was a problem hiding this comment.
raise without e is enough
| Parameters | ||
| ---------- | ||
| fileObject : an open file object | ||
| the file that contains the RepositoryCfg. |
| return | ||
|
|
||
|
|
||
| def readPafStorage(self, butlerLocation): |
| for locationString in butlerLocation.getLocations(): | ||
| logLoc = LogicalLocation(butlerLocation.getStorage().locationWithRoot(locationString), | ||
| butlerLocation.getAdditionalData()) | ||
| finalItem = Policy(filePath=logLoc.locString()) |
There was a problem hiding this comment.
I guess this Policy is different from pexPolicy? Sort of confusing.
There was a problem hiding this comment.
yeah. naming is hard. sorry.
| The formatter class that can be used by the StorageInterface instance to read and write the object | ||
| to the storage. | ||
| readFormatter : a read formatter callable | ||
| The formatter class that can be used by the StorageInterface instance to read the object from the |
There was a problem hiding this comment.
formatter class - > method? (or function if you prefer)
| cls._registerFormatter(formatable, readFormatter, 'read') | ||
|
|
||
| @classmethod | ||
| def registerWriteFormatter(cls, formatable, writeFormatter): |
There was a problem hiding this comment.
Instead three separate methods registerFormatters, registerReadFormatter, registerWriteFormatter I'd probably define just one with keyword arguments:
def registerFormatters(cls, formattable, read=None, write=None)
....
Feels more Pythonic to me 🐍
| formatter callable | ||
| The formatter callable used to read the object from the storageInterface. | ||
| """ | ||
| classFormatters = StorageInterface.readFormatters.get(cls, None) |
There was a problem hiding this comment.
One minor issue with using cls as a key in a global map — if, for example, I decide to implement new Storage as subclass of PosixStorage (not sure if that is possible) then calling self._getFormatter() in a subclass will not find any formatters registered with PosixStorage?
There was a problem hiding this comment.
yeah, it's broken. I ran into it the other day. Needs fixing.
n8pease
force-pushed
the
tickets/DM-11407
branch
3 times, most recently
from
3d781fd
to
4b2be16
Compare
instead of calling .read & .write, just have read and write formatters.
n8pease
force-pushed
the
tickets/DM-11407
branch
from
13e08ca
to
bbab5f4
Compare
n8pease
force-pushed
the
tickets/DM-11407
branch
from
3e09923
to
be307b4
Compare
No description provided.