Skip to content

Add cache repo#14

Merged
Narsil merged 3 commits into
mainfrom
add_cache_repo
Aug 16, 2023
Merged

Add cache repo#14
Narsil merged 3 commits into
mainfrom
add_cache_repo

Conversation

@Narsil

@Narsil Narsil commented Aug 16, 2023

Copy link
Copy Markdown
Contributor

Similar to what is being done for Api vs ApiRepo.

I'm hesitant to even change the internals of ApiRepo to make them own a CacheRepo.
That would remove a bunch of clone, but potential discrepancy between self.api.cache.repo(repo) vs self.cache_repo

@Narsil Narsil requested a review from McPatate August 16, 2023 09:11
@Narsil Narsil merged commit be18c65 into main Aug 16, 2023
@McPatate McPatate deleted the add_cache_repo branch August 17, 2023 09:58
@McPatate

Copy link
Copy Markdown
Member

It makes sense to me that Api owns both ApiRepo and CacheRepo and that CacheRepo is not owned by ApiRepo

@Narsil

Narsil commented Aug 17, 2023

Copy link
Copy Markdown
Contributor Author

ApiRepo owns Api not the other way around...

@McPatate

McPatate commented Aug 17, 2023

Copy link
Copy Markdown
Member

My bad, forget what I said. Why does Api have a cache repo?

Download logic is in ApiRepo &btw there's a discrepancy download_tempfile is in ApiRepo in tokio vs in Api for sync.

@Narsil

Narsil commented Aug 18, 2023

Copy link
Copy Markdown
Contributor Author

download_tempfile is private, therefore consistency doesn't matter. IIRC there's a good reason they don't need the same things.

Why does Api have a cache repo?

To get the token, and to be able to create a CacheRepo :)

I think the way to go to clean this would be to have Api be split between the Cache owned (the exposed user level API) and the actualy InternalApi which would contain only the necessary things to make actual http calls. Then

ApiRepo could own InternalApi and CacheRepo and we avoid the issue of ApiRepo owning both CacheRepo and Api.Cache (being potentially out of sync)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants