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

Do we really need readDirectory in runner.js? #172

Closed
shalithasuranga opened this issue Jul 9, 2022 · 5 comments
Closed

Do we really need readDirectory in runner.js? #172

shalithasuranga opened this issue Jul 9, 2022 · 5 comments
Labels

Comments

@shalithasuranga
Copy link
Member

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

@pathange-s
Copy link
Contributor

pathange-s commented Jul 10, 2022

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

@Sadaf-A
Copy link
Contributor

Sadaf-A commented Mar 9, 2024

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?

@abhaysinghs772
Copy link
Contributor

@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
Copy link
Contributor

@shalithasuranga, I analyzed this minor issue and I think was correct. so I raised the PR regarding this
please review it and merge it #257 .

thanks

shalithasuranga pushed a commit that referenced this issue Apr 10, 2024
* refactor: replaced runner.readDirectory by native fs.readdirSync API ( #172 )

* added .vscode in .gitignore, this is for local debugging env
@shalithasuranga
Copy link
Member Author

Fixed via #257 .. Thanks 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants