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

Parse Accept #100

Merged
merged 7 commits into from
Apr 29, 2024
Merged

Parse Accept #100

merged 7 commits into from
Apr 29, 2024

Conversation

desttinghim
Copy link
Contributor

Parses the Accept field from the header to return the ranked list of content types the client expects. I'd like feedback on the API - it doesn't seem quite right to me yet.

@desttinghim desttinghim changed the title draft: Parse Accept WIP: Parse Accept Apr 25, 2024
}){};

fn on_request_verbose(r: zap.Request) void {
const allocator = gpa.allocator();
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make sense to use a FixedBufferAllocator here? Maybe with a 1kB buffer?

src/request.zig Outdated
};

/// Parses `Accept:` http header into `list`, ordered from highest q factor to lowest
pub fn parseAccept(self: *const Self, list: *std.ArrayList(AcceptItem)) !void {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it makes sense to make a type alias for the ArrayList?
So that users can write zap.AcceptList.init() instead of std.ArrayList(zap.AcceptItem).init()?

Copy link
Member

Choose a reason for hiding this comment

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

And taking it one step further: instead of passing the ArrayList, let the user just pass an allocator and do the init within parseAccept. I think that would look very zig-like, so I propose:

pub fn parseAccept(self: *const Self, allocator: std.mem.Allocator) !std.ArrayList(AcceptItem)

Copy link
Member

Choose a reason for hiding this comment

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

Now, regarding naming: What do you think of the more verbose parseAcceptHeaders()?

pub fn parseAcceptHeaders(self: *const Self, allocator: std.mem.Allocator) !std.ArrayList(AcceptItem)

@renerocksai
Copy link
Member

BTW really nice work! I like it! I especially like the catch break stuff!

@renerocksai
Copy link
Member

^^^ I just pushed a suggestion commit to this PR

@renerocksai
Copy link
Member

^^^ that last commit was meant for master 🤣

@desttinghim desttinghim changed the title WIP: Parse Accept Parse Accept Apr 28, 2024
@desttinghim
Copy link
Contributor Author

I liked your changes, I've done a little more work on top of them. I count the number of commas up front and allocate the array list. The comma to item relationship should hold for all valid Accept headers I think.

@renerocksai
Copy link
Member

Nice! Ready to merge if you are.

@desttinghim
Copy link
Contributor Author

Lets get it merged then :)

@renerocksai renerocksai merged commit 70c0ef1 into zigzap:master Apr 29, 2024
2 checks passed
@desttinghim desttinghim deleted the parseAccept branch April 29, 2024 20:49
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