Update Resource Feature #44
Conversation
|
The message bus in |
dor/providers/minter_provider.py
Outdated
| class MinterProvider(ABC): | ||
| @abstractmethod | ||
| def mint(self) -> str: | ||
| pass |
There was a problem hiding this comment.
Minor: I'd probably have just called it Minter. It's not providing a minter, it's providing an ID, right?
| def _( | ||
| path_data: PathData, unit_of_work: AbstractUnitOfWork, message_bus: MemoryMessageBus | ||
| ): | ||
| """a package containing all the scanned pages, OCR, and metadata.""" |
There was a problem hiding this comment.
Should this comment be the same as what's in the given?
| @abstractmethod | ||
| def log(self, id: str, reversed: bool = True) -> list[VersionInfo]: | ||
| pass |
There was a problem hiding this comment.
I know the underlying command is log, but I'd like to see this method be named something more specific, like get_versions.
There was a problem hiding this comment.
It doesn't look like reversed is actually used anywhere?
There was a problem hiding this comment.
I dislike the re-using versions, because it's not versions. It's a log of versions.
There was a problem hiding this comment.
OK, I see your point, that's fine. I just wish the method said something about how it has something to do with version data. Maybe that's implied by logging an object, but feels vague at this point.
| class VersionInfo: | ||
| version: int | ||
| author: str | ||
| date: datetime |
There was a problem hiding this comment.
I'd opt for datetime or timestamp. I find it confusing when only date is used for both date and time.
There was a problem hiding this comment.
Looks as if you're using the data straight from ROCFL, so maybe this would be some work to change...
There was a problem hiding this comment.
Yes. We can't (readily) translate from the rocfl log back into discrete agent/contact, and we only care about the version number at the moment.
| def log(self, id: str, reversed: bool = True) -> list[VersionInfo]: | ||
| version_log = [] | ||
| args: list[str | Path] = ["rocfl", "-r", self.storage_path, "log", "-r", id] | ||
| info = {} | ||
| try: | ||
| result = subprocess.run(args, check=True, capture_output=True) | ||
| for line in result.stdout.decode("utf-8").split("\n"): | ||
| if not line.strip(): | ||
| if info: | ||
| version_log.append(VersionInfo(**info)) | ||
| continue | ||
|
|
||
| if line.startswith("Version "): | ||
| info = {} | ||
| info['version'] = int(line.split(" ")[-1]) | ||
| else: | ||
| key, value = [ v.strip() for v in line.strip().split(":", 1) ] | ||
| info[key.lower()] = value | ||
|
|
||
| return version_log | ||
| except CalledProcessError as e: | ||
| staged_version_not_found_message = ( | ||
| f"Not found: {id} does not have a staged version." | ||
| ) | ||
| if staged_version_not_found_message in e.stderr.decode(): | ||
| return False | ||
| raise RepositoryGatewayError() from e |
There was a problem hiding this comment.
I'd like to see tests for this and (less importantly) the FakeRepositoryGateway.log. Happy to chip in with a PR if you want me to.
|
|
||
| def log(self, id: str, reversed: bool = True) -> list[VersionInfo]: | ||
| return reversed([ | ||
| VersionInfo(version=v.number, author=v.contributor, message=v.message) |
There was a problem hiding this comment.
Does this work if you don't provide date?
There was a problem hiding this comment.
No. We'll have to fake something in the fake logging.
tests/test_workspaces/ Co-authored-by: Sam Sciolla <[email protected]>
gkostin1966
force-pushed
the
update-partial-feature
branch
from
52f44ae to
ef4377f
Compare
gkostin1966
force-pushed
the
update-partial-feature
branch
from
ef4377f to
620d241
Compare
Update resource feature with two scenarios: update everything (data and metadata) and update metadata only.