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

Zig Fmt recursive option #1068

Closed
wants to merge 1 commit into from
Closed

Zig Fmt recursive option #1068

wants to merge 1 commit into from

Conversation

BraedonWooding
Copy link
Contributor

@BraedonWooding BraedonWooding commented Jun 7, 2018

As the title says, you can call it like zig fmt src/ otherFile.zig otherStuff/inside/ -r, if you forget the -r it'll give you an error To fmt recursively pass the '-r' flag., this is to prevent people accidentally doing recursive and causing unexpected damage for whatever reason :).

Added this since I wanted to run it on a source directory of my Lazy Zig library and it was annoying to have to give 10+ source files rather than just saying zig fmt -r src/

NOTE: doesn't work on windows, since windows doesn't have directory code yet. I'll have a look in another PR if I have time to see if I can implement it.

return blk: {
const source_code = io.readFileAlloc(allocator, file_path) catch |err| {
try stderr.print("unable to open '{}': {}\n", file_path, err);
break :blk false;
Copy link
Member

Choose a reason for hiding this comment

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

You can just use a straight return for these. No need to pass return via the :blk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooops, should have realised this

@BraedonWooding
Copy link
Contributor Author

BraedonWooding commented Jun 7, 2018

@andrewrk the the failed travis check don't seem at all related to my PR maybe another issue from another PR? And the other one is because windows directories isn't implemented yet in the std (error: TODO support Dir.open for windows), this is a separate issue to this implementation.

How would you advise we approach this? I think this PR should stay open till windows support for Directories is supported in std, then it can be merged; rather than conflating windows directory support with this PR?

@andrewrk
Copy link
Member

andrewrk commented Jun 7, 2018

This PR cannot be merged until #709 is done

@BraedonWooding
Copy link
Contributor Author

When #1071 is pushed this can be merged in, I'll close and reopen it before then so the checks will re-run :).

@thejoshwolfe
Copy link
Sponsor Contributor

Recursive directory traversal is a dangerous road to go down. There are lots of features you can support in this domain that people will ask for. grep has 11 command line options for "File and Directory Selection" including features like exclude patterns, and following vs skipping symlinks.

Traversing directories is a harder problem than it seems, and I think we should leave it outside the scope of zig fmt. You can always use find to enumerate files and pass those to any other program using the -exec option.

@BraedonWooding
Copy link
Contributor Author

BraedonWooding commented Jun 7, 2018

@thejoshwolfe Almost every 'fmt'er out there supports atleast this basic recursion, for example go fmt supports this exact type of fmt I'm showing here (very simple recursive search).

And for the 'scope' of this, it really isn't that much larger; its performing an extremely simple recursive search on directories for the exact reason you gave, its a lot harder of a problem then it seems and it is easier to begin with a simple solution. I don't think using find and -exec is a solution, that seems hackish and annoying to me personally :).

A list of formatters that have this feature that I've commonly used before and from asking a few people I know all said that they commonly use;

At the very least it was motivation for me to fix dir.open for windows, so that is something; I do really think this feature definitely is useful enough to be merged

@thejoshwolfe
Copy link
Sponsor Contributor

This implementation selects files that end with .zig. That might be worth documenting.

@BraedonWooding
Copy link
Contributor Author

BraedonWooding commented Jun 13, 2018

Everything should be okay now :).

The error isn't anywhere so I'm going to close/reopen to re-run it to make sure that it is a real error.

@andrewrk
Copy link
Member

I'll look at this tomorrow morning.

@andrewrk
Copy link
Member

I haven't forgotten this - I'm part way through merging it, should have it merged tomorrow at some point.

@andrewrk andrewrk closed this in 1ca90b5 Jun 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants