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 for Process: ensure chdir is a string #13503

Merged
merged 5 commits into from
May 26, 2023

Conversation

devnote-dev
Copy link
Contributor

This fix prevents the following compile-time error by ensuring that #to_s is called for Path and String:

In C:\Users\user\Documents\Crystal\Windows\src\crystal\system\win32\process.cr:210:75

 210 | make_env_block(env, clear_env), chdir.try { |str| System.to_wstr(str) },
                                                                        ^--
Error: expected argument #1 to 'Crystal::System.to_wstr' to be String, not Path

Overloads are:
 - Crystal::System.to_wstr(str : String, name : String | ::Nil = nil)

Ensures that when `chdir` is not a `String` or `Nil` that it is converted to a `String`.
@HertzDevil HertzDevil added kind:bug A bug in the code. Does not apply to documentation, specs, etc. platform:windows Windows support based on the MSVC toolchain / Win32 API topic:stdlib:system labels May 25, 2023
@HertzDevil
Copy link
Contributor

Is there a spec for this?

@devnote-dev
Copy link
Contributor Author

No, and given that Process#run is the only method that is affected by this, I don't think that it's necessary to have specs for such a tiny issue.

@HertzDevil
Copy link
Contributor

If something doesn't compile, it isn't tiny at all

@devnote-dev
Copy link
Contributor Author

Right, but given how infrequently the process.cr file changes (ignoring documentation/formatting), and how long this issue has actually been here undetected, I don't think specs for it are really needed. Clearly it hasn't been an issue for the majority that do use it.

@HertzDevil
Copy link
Contributor

4. Any change to behaviour needs to be reflected and validated with specs.

So you are saying this bugfix doesn't deserve a spec because you belong to a minority?

@devnote-dev
Copy link
Contributor Author

No, I would make the same argument if this affected platforms other than Windows. My point is the change is so small that I think having specs for it isn't necessary. That doesn't invalidate the fact that it didn't compile beforehand, but I don't see it as something so significant that specs would be necessary for it.

@straight-shoota straight-shoota added this to the 1.9.0 milestone May 25, 2023
@oprypin oprypin changed the title fix(process): ensure chdir is a string Fix for Process: ensure chdir is a string May 25, 2023
@straight-shoota straight-shoota merged commit 1946163 into crystal-lang:master May 26, 2023
@devnote-dev devnote-dev deleted the fix-process branch May 26, 2023 21:36
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. platform:windows Windows support based on the MSVC toolchain / Win32 API topic:stdlib:system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants