code refactoring #119
code refactoring #119
Conversation
src/fs-access/file-open.mjs
Outdated
| */ | ||
| export default async (options = [{}]) => { | ||
| if (!Array.isArray(options)) { | ||
| options = [options]; |
There was a problem hiding this comment.
I'd prefer not to mutate parameters
There was a problem hiding this comment.
What do you propose as an alternative if the function should accept arrays and single values at the same time?
There was a problem hiding this comment.
const opts = Array.isArray(options) ? options : [options];
There was a problem hiding this comment.
Sure, but what would be the advantage? Honestly curious.
src/fs-access/file-open.mjs
Outdated
| const types = []; | ||
| options.forEach((option, i) => { | ||
| types[i] = { | ||
| const types = options.map((option) => { |
There was a problem hiding this comment.
Yes, this looks more “intended” indeed.
src/fs-access/file-open.mjs
Outdated
| types[i] = { | ||
| const types = options.map((option) => { | ||
| const type = { | ||
| description: option.description || 'Files', |
There was a problem hiding this comment.
I'd prefer to use destructuring assignment for options and move const type to the end of this arrow function
src/fs-access/file-open.mjs
Outdated
| } | ||
| } else { | ||
| types[i].accept['*/*'] = option.extensions || []; | ||
| type.accept['*/*'] = option.extensions || []; |
src/fs-access/file-open.mjs
Outdated
| return type; | ||
| }); | ||
| const handleOrHandles = await window.showOpenFilePicker({ | ||
| id: options[0].id, |
There was a problem hiding this comment.
Also we can use destructuring assignment for arrays and put options[0] to intermediate variable: const [first] = options; or even const [{ id, startIn, multiple, excludeAcceptAllOption }] = options;
|
Please move on with the suggested improvements and let me know when you have agreed on a design. Thanks for the PR and discussion so far already. |
|
@tomayac, @tshemsedinov, I am working on updating this file if this one is fine |
Thanks, please do. We can turn this into a big refactoring PR. |
|
@tshemsedinov, kind reminder for your code review please |
|
Is anyone still interested in this? The code as is seems to be pretty stable, so maybe we can close this? |
|
@tomayac, the refactoring is done, it just needs a proper code review. Feel free to close the PR if you do not think this code refactoring is beneficial for the library. The only purpose of this PR was to improve code readability for any possible further feature development in the library. I am willing to put more work in it if needed. |
Sounds good to me. Before I start my review, any changes planned for |
|
no changes planned for |
|
Closing as per #119 (comment). |
3 participants
This PR aims to improve code readability. No strong feelings attached. Feel free to close the PR if you think this is not necessary.