Skip to content
New issue

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

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

Already on GitHub? # to your account

fix(object): Takes into account the range of bytes starting with 0 #755

Merged
merged 7 commits into from
Oct 3, 2021

Conversation

nicoandra
Copy link
Contributor

Fixes errors when trying to re-create directories that already exist.

Uses a helper function to verify for the directory existence first.

Note this PR also includes the fix for options.start !== undefined as I've built on top of my previous branch.

Copy link
Collaborator

@kherock kherock left a comment

Choose a reason for hiding this comment

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

These look pretty good to me, do you think you could add a quick test case with the range that wasn't working before? There should be some existing tests that you can borrow from.

try {
await fs.access(bucketPath, fs.constants.F_OK);
} catch (ex) {
exists = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for refactoring this, I wasn't sure exactly where the Windows tests were going wrong since I replaced fs-extra.

Could you move this to the utils file as ensureDir(path)? You should also check that the exception code is ENOENT and rethrow when it's not. You can also move the mkdir call below directly to this catch block instead of storing a temporary variable.

@nicoandra nicoandra marked this pull request as draft June 16, 2021 14:03
@nicoandra
Copy link
Contributor Author

These look pretty good to me, do you think you could add a quick test case with the range that wasn't working before? There should be some existing tests that you can borrow from.

Hey @kherock , the ranges changes were done in a separate PR: #753

I'll revisit this PR again to refactor a few more things and get back to you. Thanks!

@nicoandra nicoandra marked this pull request as ready for review June 16, 2021 20:57
@nicoandra nicoandra requested a review from kherock June 16, 2021 20:57
@codecov
Copy link

codecov bot commented Jun 17, 2021

Codecov Report

Merging #755 (ea62ef1) into main (1cdcb85) will decrease coverage by 0.03%.
The diff coverage is 92.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #755      +/-   ##
==========================================
- Coverage   89.53%   89.49%   -0.04%     
==========================================
  Files          22       22              
  Lines        1471     1476       +5     
==========================================
+ Hits         1317     1321       +4     
- Misses        154      155       +1     
Impacted Files Coverage Δ
lib/utils.js 95.06% <80.00%> (-1.00%) ⬇️
lib/stores/filesystem.js 93.53% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fee5d99...ea62ef1. Read the comment docs.

@kherock kherock changed the title Fix/mkdir fix(object): Takes into account the range of bytes starting with 0 Jun 17, 2021
Copy link
Collaborator

@kherock kherock left a comment

Choose a reason for hiding this comment

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

This looks good, thanks. I'll merge and backport to 3.x when I have time.

@non-senses
Copy link
Contributor

This PR is related to this issue: #754

@kherock kherock merged commit e90381c into jamhall:main Oct 3, 2021
kherock pushed a commit that referenced this pull request Oct 3, 2021
)

* adds tests for requests without ranges and ranges of 0-(null)

Co-authored-by: Nico Andrade <nicolas.andrade@ssense.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants