Skip to content

Rewrite in Zig#144

Open
jstrieb wants to merge 73 commits intomasterfrom
zig
Open

Rewrite in Zig#144
jstrieb wants to merge 73 commits intomasterfrom
zig

Conversation

@jstrieb
Copy link
Copy Markdown
Owner

@jstrieb jstrieb commented Mar 29, 2026

Before merging, I will need to do some history revision to clean things up. One of the changes on this branch moves the generated files to their own branch, and I will need to manually fix some stuff up in accordance with that.

@@ -0,0 +1,230 @@
const std = @import("std");

// Since parse is the only public function, these variables can be set there and
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

set here*?

}
}

fn strip_optional(T: type) type {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why use snake case for these?

const value = try a.dupe(u8, entry.value_ptr.*);
defer a.free(value);
@field(result, field.name) = value.len > 0 and
!std.ascii.eqlIgnoreCase(value, "false");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

didn't know about this function, nice

) !void {
inline for (@typeInfo(T).@"struct".fields, seen) |field, *seen_field| {
if (!seen_field.*) {
if (field.defaultValue()) |default| {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ah nice, I only ever used default_value_ptr, this is better

.pointer => @field(
result,
field.name,
) = if (default) |p| try allocator.dupe(u8, p) else null,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why bother dupe-ing?

Comment on lines +208 to +210
const flag_version = try allocator.dupe(u8, field.name);
defer allocator.free(flag_version);
std.mem.replaceScalar(u8, flag_version, '_', '-');
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A comptime version of dupe would be very handy. I basically made one in my argparser

try stdout.print("--{s}\n", .{flag_version});
},
else => {
const flag_version = try allocator.dupe(u8, field.name);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

did you mean to call it flag_ in this else branch?

var env = try std.process.getEnvMap(a);
defer env.deinit();
var iterator = env.iterator();
while (iterator.next()) |entry| {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

interesting, I did the reverse where I just looped through each field, generated the env key and did env.get() on it

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

might be slightly simpler

Comment on lines +16 to +17
const arena = try allocator.create(std.heap.ArenaAllocator);
arena.* = std.heap.ArenaAllocator.init(allocator);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why not var arena = std.heap.ArenaAllocator.init(allocator);?

}

var writer = try std.Io.Writer.Allocating.initCapacity(
self.arena.allocator(),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is an arena really the right model here? I imagine you'll make a lot of http requests whose responses are fairly disposable, but whose memory footprint will grow the arena over time without freeing anything until very late in the program lifespan.

.extra_headers = extra_headers,
}) catch |err| switch (err) {
error.HttpConnectionClosing => {
// Handle a Zig HTTP bug where keep-alive connections are closed by
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

unfortunate, but the workaround makes sense

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I imagine this (and perhaps other parts of the code) prevents you from running multiple requests in parallel

);
errdefer writer.deinit();
const status = (try (self.client.fetch(.{
.location = .{ .url = url },
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

shouldn't you specify .method = POST here?

.payload = body,
.headers = headers,
}) catch |err| switch (err) {
error.HttpConnectionClosing => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

personally I would be making a private inner version of get/post since these two functions are so similar

Comment on lines +66 to +74
if (self.api_key) |s| allocator.free(s);
if (self.json_input_file) |s| allocator.free(s);
if (self.json_output_file) |s| allocator.free(s);
if (self.excluded_repos) |s| allocator.free(s);
if (self.excluded_langs) |s| allocator.free(s);
if (self.overview_output_file) |s| allocator.free(s);
if (self.languages_output_file) |s| allocator.free(s);
if (self.overview_template) |s| allocator.free(s);
if (self.languages_template) |s| allocator.free(s);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

could always do an inline for on the fields :)


fn overview(
arena: *std.heap.ArenaAllocator,
stats: anytype,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

it's interesting, you seem to prefer anonymous structs much more than me. I would probably have a single Overview struct instead of accepting anytype. I generally think type-first which could be seen as a limitation in languages where I can't annotate as heavily as I may want.

\\
, .{ (i + 1) * 150, color orelse "#000", language, percent });
}
// Vulnerable to template injection. In practice, this should never happen.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Quickly looked through the possibilities, seems as though nobody but the author can really affect the contents here in a meaningful way.

try list.append(allocator, pattern);
}
break :excluded try list.toOwnedSlice(allocator);
} else null;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why do you need the else null?

} else null;
defer if (excluded_langs) |excluded| allocator.free(excluded);

var stats: Statistics = undefined;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why use the = undefined pattern here but the labeled breaks above?

) ![]const u8 {
const a = arena.allocator();
var out_data = template;
// Vulnerable to template injection. In practice, this should never happen.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Another idea is to just use std.fmt.print. So convert your templates to use {[lang_list]s} syntax and you can just do std.fmt.print and pass the stats struct directly

Comment on lines +324 to +327
if (std.mem.eql(u8, path, "-"))
std.fs.File.stdin()
else
try std.fs.cwd().openFile(path, .{});
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I like this formatting, I wonder if I can get zls to do it

@@ -0,0 +1,558 @@
const std = @import("std");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Image

GitHub, I'm coming for you.

fn get_basic_info(
client: *HttpClient,
allocator: std.mem.Allocator,
) !struct { []u32, []const u8, ?[]const u8 } {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

personally it would bother me not having these named for the caller

Comment on lines +516 to +518
item.delay +=
std.crypto.random.intRangeAtMost(i64, 2, item.delay);
item.delay = @min(item.delay, 600);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I like this :)

Comment on lines +512 to +513
.ok => {},
.accepted => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this works out pretty nicely


const limit = 100;
if (stats.commitContributionsByRepository.len >= limit) {
for (&[_]usize{ 2, 3 }) |factor| {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

hmm so this requires that the initial caller passes a number of months whose prime factors are 2 and 3, right?

}
return;
}
} else {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

so this would be hit when months = 1 and the limit is still exceeded? Interesting, I guess this works

Comment on lines +351 to +356
errdefer {
for (0..i, repository.languages.?) |_, l| {
context.allocator.free(l.name);
if (l.color) |c| context.allocator.free(c);
}
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[question] what if you hit an error on line 374? This errdefer will have gone out of scope, right? so these frees don't run in that case?

.{ .ignore_unknown_fields = true },
)).count;
} else {
std.log.info(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Would expect this to be a warn

}
allocator.free(result.repositories);
}
std.sort.pdq(Repository, result.repositories, {}, struct {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

haven't had use for any of the stdlib sorting functions yet, but this is good to know about

}
}{ .values = aggregate_stats.languages.values() });

{
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why is this in a bock?

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.

2 participants