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

Compile-time routes definition #19

Merged
merged 15 commits into from
Dec 18, 2024
Merged

Conversation

Madeorsk
Copy link
Contributor

As discussed in #18, here is a pull request for compile-time routes definition.

I tried to keep exactly the same features, but keeping per-route custom data wasn't possible because routes couldn't take runtime values anymore. I created a state variable in context, with UserState type, set on server type definition.

There are also 2 things that I'd like to change, that I'll describe in comments.

…outer parts in router directory.

+ Define routes at compile-time.
+ Add router user data, usable from context.
* Adapt routes for fully compile-time definitions, without runtime data handling.
* Change serve_fs_dir to use resolve instead of realpath (see ziglang/zig#19353).
- Remove per-route user data.

WIP: filesystem serve.
src/http/router.zig Outdated Show resolved Hide resolved
@mookums
Copy link
Owner

mookums commented Dec 15, 2024

Hi, thanks for submitting this! I've been busy recently but I'll take a look at in the coming days!

@mookums mookums self-assigned this Dec 15, 2024
@mookums mookums added the enhancement New feature or request label Dec 15, 2024
Copy link
Owner

@mookums mookums left a comment

Choose a reason for hiding this comment

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

This is pretty solid implementation wise and I'm pretty happy about this so good job! I left some notes about naming just because I think it's important for ease of maintainability. I also appreciate all of the documentation and examples being updated along with the code :)

I should probably formalize this somewhere but the style I try to use is:

  • snake_case for variables and methods on structs. (This is one that came up the most, not a giant issue just a style one).
  • '_namefor types that need compile time parameters. (You were fine on this aside from that one_self`)
  • Avoid file as struct.
  • Zig Style Guide for the rest.

src/http/server.zig Outdated Show resolved Hide resolved
src/http/router/token_hash_map.zig Outdated Show resolved Hide resolved
src/http/router/token_hash_map.zig Outdated Show resolved Hide resolved
src/http/router/route.zig Outdated Show resolved Hide resolved
src/http/context.zig Outdated Show resolved Hide resolved
@Madeorsk
Copy link
Contributor Author

snake_case for variables and methods on structs. (This is one that came up the most, not a giant issue just a style one).

Tbh, I noticed it, I did that to see how important was this naming convention to you. My opinion is that it's a bit surprising to use this naming convention when all Zig is made different, but I'll change my code to use snake_case!

@Madeorsk Madeorsk requested a review from mookums December 16, 2024 23:35
@mookums mookums merged commit c3e26ed into mookums:main Dec 18, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants