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

Expand roc format to support directories #7703

Merged
merged 1 commit into from
Mar 15, 2025

Conversation

bhansconnect
Copy link
Member

Also supports formatting multiple passed in args.
Also prints the overall execution time.

smores56
smores56 previously approved these changes Mar 15, 2025
Copy link
Collaborator

@smores56 smores56 left a comment

Choose a reason for hiding this comment

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

No blocking comments, so this is good to merge, mainly questions we should answer at some point.

/// Formats all roc files in the specified path.
/// Handles both single files and directories
/// Returns the number of files formatted.
pub fn formatPath(gpa: std.mem.Allocator, base_dir: std.fs.Dir, path: []const u8) !usize {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not important to integrate with yet, but we have a half-baked filesystem wrapper that will help us with supporting WASI and maybe testing. That would probably be good to discuss the merits of in Zulip before we get too far.

Copy link
Member Author

Choose a reason for hiding this comment

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

To note, this isn't really changed. I just moved this code. But I agree we should fix it up.

src/fmt.zig Outdated
for (parse_ast.errors) |err| {
try stderr.print("\t{s}\n", .{@tagName(err.tag)});
}
fatal("\n", .{});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on what I see in the old compiler, this "exit on malformed code" seems to mostly match our current behavior, but I don't know if it's desired to fail if there are literally any parsing problems. What if there's a single unfinished function body, but everything else is valid?

Did I miss a discussion on whether this was what we wanted? It's fine for now, but we'll want to make sure we align here at some point.

Copy link
Member Author

Choose a reason for hiding this comment

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

Long term the goal is that most errors will be recoverable and get formatted. For now, that is not anywhere near the case. So we just fail.

@bhansconnect bhansconnect disabled auto-merge March 15, 2025 08:35
@bhansconnect bhansconnect enabled auto-merge March 15, 2025 08:35
@bhansconnect
Copy link
Member Author

I'll make a follow up tomorrow

@isaacvando
Copy link
Collaborator

@bhansconnect Looks good! Would you mind writing some tests?

@bhansconnect
Copy link
Member Author

Would you mind writing some tests?

I don't think we have a good way to test this until we have proper mocks for the filesystem. That or at a minimum we need to spin up a proper temp directory. The current test generates a file in the roc project folder. I don't want to make more tests like that.

@bhansconnect
Copy link
Member Author

Haha, tests are hanging cause you are not allowed to print to stdout in a zig test.... So top level functions that print to the terminal through stdout aren't actually testable in zig.

I forgot this was a restriction of zig build test cause it uses stdout to communicate.

@bhansconnect bhansconnect force-pushed the bhansconnect/push-lqvyptntrtqs branch 3 times, most recently from c4f86b4 to b923627 Compare March 15, 2025 19:08
Also supports formatting multiple passed in args.
Also prints the overall execution time.
@bhansconnect bhansconnect force-pushed the bhansconnect/push-lqvyptntrtqs branch from b923627 to 68dfd00 Compare March 15, 2025 19:10
@bhansconnect bhansconnect disabled auto-merge March 15, 2025 21:13
@bhansconnect bhansconnect merged commit 6511c9b into main Mar 15, 2025
18 checks passed
@bhansconnect bhansconnect deleted the bhansconnect/push-lqvyptntrtqs branch March 15, 2025 21:13
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.

3 participants