Skip to content

refactor: clean up file system entry retrieval#112

Merged
rolandjitsu merged 1 commit intoreact-dropzone:betafrom
jonkoops:promisify-reader
Nov 22, 2024
Merged

refactor: clean up file system entry retrieval#112
rolandjitsu merged 1 commit intoreact-dropzone:betafrom
jonkoops:promisify-reader

Conversation

@jonkoops
Copy link
Copy Markdown
Collaborator

@jonkoops jonkoops commented Nov 11, 2024

What kind of change does this PR introduce?

  • Bug Fix
  • Feature
  • Refactoring
  • Style
  • Build
  • Chore
  • Documentation
  • CI

Did you add tests for your changes?

  • Yes, my code is well tested
  • Not relevant

If relevant, did you update the documentation?

  • Yes, I've updated the documentation
  • Not relevant

Summary
Cleans up the code that retrieves and processes file system entries so that the flow is easier to follow. This is done by wrapping methods such as readEntries() and file() and 'promisifying' their callback style to promises, which allows them to be awaited.

Additionally, this PR re-structures the fromDirEntry() function to use the above and uses a while loop to drain the directory reader until there are no more entries to read. This simplifies the code and improves readability as it is no longer required to jump through various functions and manually manage the promise with resolve() and reject() calls.

Type safety is also improved by adding proper type definitions to all the affected code.

Does this PR introduce a breaking change?
No

Other information
This is a cleanup, but also I am going through and selectively re-writing code to get more familiar with the code-base.

@coveralls
Copy link
Copy Markdown

coveralls commented Nov 11, 2024

Pull Request Test Coverage Report for Build 11821216180

Details

  • 26 of 26 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 11817735531: 0.0%
Covered Lines: 89
Relevant Lines: 89

💛 - Coveralls

@jonkoops
Copy link
Copy Markdown
Collaborator Author

Note this is just the beginning of the code cleanup, I'll be going through the code and refactoring it before releasing the new major so I have a better understanding of how the code works in case bugs arise.

@jonkoops
Copy link
Copy Markdown
Collaborator Author

@rolandjitsu this one is ready to be reviewed, can I ask you to take a look at it?

@jonkoops
Copy link
Copy Markdown
Collaborator Author

I see the coverage went down because the tests are not hitting a specific no-op case that did not exist before, let me add a test for that.

@jonkoops jonkoops marked this pull request as draft November 11, 2024 18:01
@jonkoops
Copy link
Copy Markdown
Collaborator Author

Ok, test added. Coverage is back at a 100%.

@jonkoops jonkoops marked this pull request as ready for review November 11, 2024 18:29
Copy link
Copy Markdown
Collaborator

@rolandjitsu rolandjitsu left a comment

Choose a reason for hiding this comment

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

The simplification of the directory traversal and the file entry reading is quite nice. Thanks for that. But the function calls are unnecessary.

Signed-off-by: Jon Koops <jonkoops@gmail.com>
@jonkoops
Copy link
Copy Markdown
Collaborator Author

@rolandjitsu I agree that isFileSystemDirectoryEntry() and isFileSystemFileEntry() were too overzealous in naming so I've simplified them to isDirectoryEntry() and isFileEntry(), that should be plenty of information since they are always called in a context.

@rolandjitsu
Copy link
Copy Markdown
Collaborator

@jonkoops I did leave some comments. Whenever you've got the time, have a look.

@jonkoops
Copy link
Copy Markdown
Collaborator Author

Will do, I've been busy with some other tasks but I will make some time.

@jonkoops
Copy link
Copy Markdown
Collaborator Author

@rolandjitsu left a reply to your comments (#112 (comment)) and resolved some of the other threads, as those were kind of becoming overlapping discussions about the same point.

@rolandjitsu rolandjitsu merged commit 1a997bd into react-dropzone:beta Nov 22, 2024
@jonkoops jonkoops deleted the promisify-reader branch November 22, 2024 11:52
@jonkoops
Copy link
Copy Markdown
Collaborator Author

Thanks again for the thorough review @rolandjitsu!

@github-actions
Copy link
Copy Markdown

🎉 This PR is included in version 3.0.0-beta.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jonkoops jonkoops restored the promisify-reader branch November 27, 2024 16:18
@jonkoops jonkoops deleted the promisify-reader branch November 27, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants