Repository navigation
Leaky abstraction in OpenPechFS's path arg #226
Description
How it's leaky abstraction?
output_path is the path to store all the pechas created by the formatters. It defaults to ~/.openpecha/pechas/
But in OCRFormatter's create_opf, the output_path is passed as path of OpenPecha which is opf_path
now, the caller code, eg OCR-pipelines, needs to create opf_path for pecha, which in turn requires to create pecha_id. But the pecha_id generation is handled by Metadata, which is only created in the Formatters. So, with currently implementation, pecha will be saved at opf_path created by caller code. Since, it's doesn't have access to Metadata creation, metadata will generate new pecha_id. Now, we ended up with different, pecha_id in opf_path and `meta.yml.
Therefore, I think this is a leaky abstraction. The caller code only needs to provide where to store the all the pechas to the Formatters.
Why this problem exists?
I think, there is two scenarios, creating and loading pecha and we don't have clean way to handle these two.
Solution
1. When Creating new pechas
We should initialise the pecha object with the actual data like, base, layers, metadata, etc.
When saving, we should provide output_path which is the parent path of the pecha path like {output_path}/{pecha_id}/{pecha_id}.opf. Now the output_path is configurable, which is desired behaviour.
class OpenPechaFS(OpenPecha):
...
@property
def opf_path(self):
return self.base_path / self.pecha_id / f"{self.pecha_id}.opf"
def save(output_path: Path) -> Path:
self.base_path = output_path
...
return self.opf_path2. When Loading existing pechas
I think we should go with classmethods, for eg:
pecha = OpenPechaFS.from_path(<path_to_pecha>) # loads local pechapecha = OpenPechaFS.from_id(<pecha_id>) # downloads pecha from githubpecha = OpenPechaGitRepo.from_commit_sha(<commit_sha>) # loads pecha from specific commit shaMetadata
Metadata
Type
Fields
Priority
Activity
eroux commentedon Dec 26, 2022
I'm not sure about the first idea, I think you're wanting to make
OpenPechaFSa very high level class, but I think it's not a good idea. I think we need a class that interacts with a local.opffolder and that's it, I think the current state ofOpenPechaFSis just that so I wouldn't change it.Now, I also agree we need a high-level class that reserves an ID, create the repo on github, downloads it, etc. and that's OpenPechaGitRepo. So I would instead do:
Note that if we add the
revargument, the path on disk should be changed toself.base_path / self.pecha_id + "_" + self.rev / self.pecha_id + ".opf". Otherwise you're going to get bugs when opening one pecha at two different revisions (since they'll share the same folder). I think it's a good thing overall, good idea!Also, I think the creation of a new openpecha id should not be hidden deep in the
Metadatageneration, it should be a static method ofOpenPechaGitRepothat anyone can use:Note that in ocr_formatter you can then use
OpenPechaGitRepo.reserve_new_id()to pass thepecha_idargument to create_opf, I think that's a small thing you can do that should solve your current issueI think having
output_pathin the__init__ofOCRFormatteris not ideal, it would be better to haveopf_pathas an argument ofcreate_opf. That way theOCRFormatterdoesn't need to deal with creating a new ID, cloning github repos, etc. which I think shouldn't be its job, it's just there to write some files in an opf.And finally, I think it would be much cleaner if we had a method
create_in_opmethod inOCRFormatter. That way everything is abstracted correctly, we already have ourOpenPechaobject in the function, it can be of any subclass ofOpenPecha, that's the cleanest solution.What do you think?
10zinten commentedon Dec 30, 2022
so, that means, the caller/client code using the
Formatterhas to make prepareopf_path?