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

Feature/fix id in route attribute #36

Merged
merged 3 commits into from
Oct 17, 2023

Conversation

TomaszCzyz
Copy link
Contributor

When I was playing around with the client, I noticed a silly mistake in a RouteAttribute with id path parameter.
The correct format for a route is [Route("/{id}")] (the previous version was not catching the parameter [Route("/:id")]).

Another option would be to merge the GetAll and Get methods by applying the ? syntax for optional parameters.

[HttpGet]  
[Route("/")]  
[Route("/{id?}")]  
public Task<List<WeatherForecast>> GetAll(string? id, CancellationToken cancellationToken)
{
	// ...
}

However, for the sake of a clearer example, I would not do that :)

@Odonno
Copy link
Contributor

Odonno commented Oct 15, 2023

Hi @TomaszCzyz

Oh yes, you are right. My bad. Can you also update the readme file? There is an example code that displays this Controller.

And I would like to stick with 2 separate endpoints for GetAll and GetById. Like you said, it is clearer that way.

@TomaszCzyz
Copy link
Contributor Author

Hi @Odonno README updated 😉

@Odonno
Copy link
Contributor

Odonno commented Oct 15, 2023

Nice. Looks good to me.

@TomaszCzyz TomaszCzyz closed this Oct 15, 2023
@TomaszCzyz TomaszCzyz reopened this Oct 15, 2023
@TomaszCzyz
Copy link
Contributor Author

Hi @Odonno, should I mention someone or can you approve the workflow to merge the PR?

@Odonno
Copy link
Contributor

Odonno commented Oct 16, 2023

@kearfy is your hero.

@kearfy kearfy merged commit 7e095b0 into surrealdb:main Oct 17, 2023
2 checks passed
@TomaszCzyz TomaszCzyz deleted the feature/fix_id_in_route_attribute branch October 17, 2023 08: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