neutralinojs-cli icon indicating copy to clipboard operation
neutralinojs-cli copied to clipboard

Do we really need readDirectory in runner.js?

Open shalithasuranga opened this issue 2 years ago • 1 comments

I am not sure why we need to use the existing readDirectory function by re-constructing the output of fs.readdirSync. Can we use the fs API directly in the test file without the wrapper function?

Assignee @pathange-s

shalithasuranga avatar Jul 09 '22 08:07 shalithasuranga

There might arise a case surely in the future where we will need to get the files present in the directory. So, I thought of creating a separate function for that. I feel having it this way keeps the code modular. Let me know.

For example, check out this. We are calling readDirectory a couple of times. Removing this will lead to writing duplicate code. https://github.com/neutralinojs/neutralinojs-cli/blob/master/spec/build.spec.js

pathange-s avatar Jul 10 '22 19:07 pathange-s

There might arise a case surely in the future where we will need to get the files present in the directory. So, I thought of creating a separate function for that. I feel having it this way keeps the code modular. Let me know.

For example, check out this. We are calling readDirectory a couple of times. Removing this will lead to writing duplicate code. https://github.com/neutralinojs/neutralinojs-cli/blob/master/spec/build.spec.js

Hey @shalithasuranga, considering this is this issue still up for contributions?

Sadaf-A avatar Mar 09 '24 09:03 Sadaf-A

@shalithasuranga and @Sadaf-A , I also agree with the suggestions made above by @pathange-s, although yes we can use fs API directly.

please do let me know about your suggestions, I will do the needful.

abhaysinghs772 avatar Apr 01 '24 17:04 abhaysinghs772

@shalithasuranga, I analyzed this minor issue and I think was correct. so I raised the PR regarding this please review it and merge it https://github.com/neutralinojs/neutralinojs-cli/pull/257 .

thanks

abhaysinghs772 avatar Apr 01 '24 20:04 abhaysinghs772

Fixed via #257 .. Thanks :tada:

shalithasuranga avatar Apr 10 '24 16:04 shalithasuranga