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

HTTP dynamic path segments #19

Merged
merged 6 commits into from
Aug 31, 2024

Conversation

seionmoya
Copy link
Collaborator

@seionmoya seionmoya commented Aug 31, 2024

The code is functional and works, marked it WIP for discussion.

Inspired by #18, I implemented HTTP path arguments into FuyuServer.
Implementation differs from @nexus4880's version by not using LINQ and using multi-threading for lookup to offset the cost of segment matching.

Discussion

I'm not happy with setting path inside FuyuBehaviour's constructor. Suggestions and ideas are welcome!
Since FuyuServer contains a unique identifier (FuyuServer.Name), maybe we can bind FuyuBehaviour on runtime to FuyuServer by using a new attribute [FuyuRoute(string serverName, string path)] and initialize FuyuBehaviour using it?

@seionmoya seionmoya added the enhancement New feature or request label Aug 31, 2024
@seionmoya seionmoya self-assigned this Aug 31, 2024
Copy link
Contributor

@nexus4880 nexus4880 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 definitely a far more elegant solution than what I had written.

I rather like the usage of the constructor holding the path as the registration of the behaviour is as simple as FuyuServer.AddHttpService<FuyuBehaviour>(); instead of having the path in the function call.

I think an attribute could be looked into later. As far as adding dynamic paths I really feel like this hits the mark and should be merged.

Lacyway
Lacyway previously approved these changes Aug 31, 2024
Copy link
Member

@Lacyway Lacyway left a comment

Choose a reason for hiding this comment

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

If this works as I think, I agree with nexus 100%. Looks very clean to me.

@seionmoya seionmoya changed the title WIP: http path arguments HTTP dynamic path segments Aug 31, 2024
@seionmoya seionmoya merged commit e0bfbb0 into project-fika:main Aug 31, 2024
1 check passed
@seionmoya seionmoya deleted the fuyubehaviour-arguments branch August 31, 2024 21:59
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.

3 participants