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

fix: tolerate space when parsing file directory #136

Merged
merged 2 commits into from
Apr 29, 2023

Conversation

jazelly
Copy link
Contributor

@jazelly jazelly commented Apr 22, 2023

Copy link
Contributor

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

Thanks for looking into it!

I think the project needs a new unit test for this fix

src/index.js Outdated Show resolved Hide resolved
@jazelly
Copy link
Contributor Author

jazelly commented Apr 22, 2023

Hi @Eomm thanks for the review.

I tried adding unit test to this but it's not straight forward.

The best I can think of is to test chile_process.spawn() to see if it accepts double quotes, but it obeys the point of unit testing, i.e. it it not testing tool.collect() in this project but other outside modules.

@jazelly jazelly requested a review from Eomm April 22, 2023 22:34
Copy link
Contributor

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

It's better to do it directly on .spawn. Also, how does it partially fix the issue? what's missing?

@jazelly
Copy link
Contributor Author

jazelly commented Apr 22, 2023

@RafaelGSS

It's better to do it directly on .spawn

Thanks. I'll add the unit test.

what's missing?

I mentioned that since this PR only aims fixing the space issue, but looks like the ECONNREFUSED is not an issue of this repo.

Also, windows stripped out \ in the path. Will fix that as well.

@jazelly
Copy link
Contributor Author

jazelly commented Apr 23, 2023

I messed up with my email config in previous commits. Had to squashed the commits.

I understand that might cause some difficulty to review the changes, but please have another look. CI should be happy now.

test/basic.test.js Outdated Show resolved Hide resolved
Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>
@jazelly jazelly requested a review from Eomm April 24, 2023 22:03
Copy link
Contributor

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

I have no power here, but LGTM!
Thks!

@jazelly
Copy link
Contributor Author

jazelly commented Apr 28, 2023

Hi @RafaelGSS, do you think this can be merged?

@RafaelGSS RafaelGSS merged commit 91fe729 into clinicjs:main Apr 29, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants