-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
zig init: allow specifying project path #24298
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
base: master
Are you sure you want to change the base?
Conversation
I need to preface this with the fact that I am not involved in any development of Zig in any way. I am looking at this PR from the perspective of just a programmer. Just by looking over your changes, I wonder if it be more readable to make the 'path' variable an optional u8 slice instead of relying on a separate 'path_specified' boolean variable and the u8 slice for the path? My thought process here is that it is less separate variables to have to be thinking about, and the path is either preset and valid or null and not specified. Further more, looking over the code to make the directory, if one was provided, I believe you should return an error if the directory already exists. By not returning an error, the user would assume nothing went wrong and the project correctly initialized in the directory they specified. I suppose you could implement further logic to check if the directory is empty and still initialize the project, but I'm not sure about that one. Just some food for thought. |
if (path != null) { | ||
fatal("more than one path specified", .{}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will fail before the first assignment because path
starts as an empty slice. Did you test these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I have a problem with building zig on my computer so I couldn't test it. I'll convert this to a draft and request reviews once the problem is resolved.
switch (e) { | ||
error.PathAlreadyExists => return, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to be an error instead. Someone might not want to accidentally writing any Zig files in their existing directory
This PR allows
zig init
to initialize project when path is specified.Usage:
When specified path already exists as non-dir, the cli throws fatal()
Closes: #24290