Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Little beautification of image cache mgmt code #2063

Merged
merged 6 commits into from
Jul 22, 2024
Merged

Conversation

kelson42
Copy link
Collaborator

This is mostly clarification and documentation around the image (cache) mgmt.

@kelson42 kelson42 requested a review from audiodude July 15, 2024 18:38
@kelson42 kelson42 added this to the 1.14.0 milestone Jul 15, 2024
@kelson42
Copy link
Collaborator Author

@audiodude This PR has two objectives:

  • Clarify a bit what is going on around image download/update (with the cache)
  • Letting you having a look to it to check it looks good (before we refill the cache). I believe the block mwResp.status === 304 could be done better but don't know how :(

@kelson42 kelson42 changed the title Clean wepb in s3 Little beautification of image cache mgmt code Jul 15, 2024
Copy link
Member

@audiodude audiodude left a comment

Choose a reason for hiding this comment

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

LGTM

@kelson42
Copy link
Collaborator Author

@audiodude There is no way tomdo better for the second bukket point?

@kelson42
Copy link
Collaborator Author

@audiodude Do you in particular could explain to me please what does this line? Does it copy only the headers or does it copy the body as well?
https://github.com/openzim/mwoffliner/pull/2063/files#diff-b37958e03ae0b3fd7818115e6506d5ac5e8e10df266bee9ce4d1606838d83bc2R564

@audiodude
Copy link
Member

audiodude commented Jul 19, 2024

@audiodude Do you in particular could explain to me please what does this line? Does it copy only the headers or does it copy the body as well? https://github.com/openzim/mwoffliner/pull/2063/files#diff-b37958e03ae0b3fd7818115e6506d5ac5e8e10df266bee9ce4d1606838d83bc2R564

I believe it copies everything but the body.

(({ Body, ...o }) => o)(s3Resp)

So it's creating an anonymous function that takes an object, and destructures it into two parts, Body and ...o which I believe is "everything else". Then it returns the everything else. This anonymous function is immediately called on the s3Resp object.

@audiodude
Copy link
Member

Just wondering, do you want me to correct all the little grammatical mistakes in the comments?

@kelson42
Copy link
Collaborator Author

kelson42 commented Jul 19, 2024

Just wondering, do you want me to correct all the little grammatical mistakes in the comments?

You can obviously. Anything to improve code base is always welcome.

Copy link

codecov bot commented Jul 19, 2024

Codecov Report

Attention: Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 74.36%. Comparing base (6c919b0) to head (6dd1c5c).

Files Patch % Lines
src/S3.ts 25.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2063      +/-   ##
==========================================
- Coverage   74.44%   74.36%   -0.08%     
==========================================
  Files          41       41              
  Lines        3146     3144       -2     
  Branches      689      688       -1     
==========================================
- Hits         2342     2338       -4     
- Misses        684      685       +1     
- Partials      120      121       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kelson42 kelson42 merged commit acedee6 into main Jul 22, 2024
3 checks passed
@kelson42 kelson42 deleted the clean-wepb-in-s3 branch July 22, 2024 16:26
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