Skip to content

Conversation

@isoos
Copy link
Collaborator

@isoos isoos commented Oct 28, 2024

Continuing #8183.

@isoos isoos requested review from jonasfj and sigurdm October 28, 2024 15:22
if (info == null) {
return ContentMatchStatus.missing;
}
// TODO: implement quick md5 match that doesn't require to download full content
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds important. Perhaps open an issue and attach the issue number here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is more of a shortcut on repeated upload checks than a critical part, because it would only help the rejection path, which may be rare. Instead, I think we should move all of these checks into a separate isolate and not do it in the frontend-serving one as part of the direct request path. I'll file an issue for that and add this to it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the code to do the md5 hash check already with this refactor. PTAL.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Contributor

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the title of the PR is wrong - there seems to be going more on than just moving code...

@isoos
Copy link
Collaborator Author

isoos commented Oct 29, 2024

I think the title of the PR is wrong - there seems to be going more on than just moving code...

The logic of the method did not change, only a return value was introduced, because it has three result states, which was handled inlined previously. I can separate it out in a new PR if you prefer that.

@sigurdm
Copy link
Contributor

sigurdm commented Oct 29, 2024

You can also just update the title - "moving" code to me means you can more or less find each deleted line added somewhere else.

@isoos isoos changed the title Moving more methods into PackageStorage. Refactoring more methods into PackageStorage. Oct 29, 2024
@isoos isoos merged commit 9071b4c into dart-lang:master Oct 29, 2024
32 checks passed
@isoos isoos deleted the storage-refactor branch October 29, 2024 12:09
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