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

Exposing Find #46

Merged
merged 2 commits into from
Dec 27, 2017
Merged

Exposing Find #46

merged 2 commits into from
Dec 27, 2017

Conversation

jackwhelpton
Copy link
Contributor

I raised a ticket to discuss this proposal about a month ago:

#42

The change is very slight, just exposing the Find method on the router. The purpose of this is to allow an API to map an internal or "self" link to a database entity, by determining the ID of the object that link represents.

I've also moved the Find method up in its file to keep all the public methods together.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.034% when pulling b189791 on jackwhelpton:master into efb54ad on go-ozzo:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.034% when pulling b189791 on jackwhelpton:master into efb54ad on go-ozzo:master.

@qiangxue
Copy link
Member

qiangxue commented Dec 27, 2017

Instead of directly exposing find, I think we should wrap it with a new public method to have a more meaningful function signature, something like this

Find(method, path string) (handlers []Handler, params map[string]string)

@coveralls
Copy link

coveralls commented Dec 27, 2017

Coverage Status

Coverage increased (+0.06%) to 94.095% when pulling 44ec0bc on jackwhelpton:master into efb54ad on go-ozzo:master.

@jackwhelpton
Copy link
Contributor Author

That makes a lot of sense; I've done that in the latest commit. Let me know if there are any other changes you'd like to see.

As a follow-up question, I'm actually working with the fasthttp-routing adaptation of ozzo-routing, so I'd like to get this change over to there too. I could make a separate PR to re-implement this small change, but I wonder if it might make more sense to make a fresh fork of ozzo-routing and re-introduce the fasthttp changes there, as the current fasthttp-routing looks to be several years behind what's here?

I can fork ozzo again into my personal Github and just make those changes there (i.e. github.com/jackwhelpton/fasthttp-routing), but would you have a preferred method for updating the current version?

@qiangxue qiangxue merged commit 3b22ec8 into go-ozzo:master Dec 27, 2017
@qiangxue
Copy link
Member

Thanks!

Regarding fasthttp-routing, yes it's outdated (far behind from both ozzo-routing and fasthttp). So it makes sense to fork from the latest ozzo-routing and incorporates the latest fasthttp. I think it makes sense to keep it under your personal github.

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