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

trails_handler supports parameter #101

Merged
merged 6 commits into from
Jul 27, 2021
Merged

Conversation

turtleDeng
Copy link
Contributor

@turtleDeng turtleDeng commented Jul 26, 2021

cowboy_trails is a great project.
trails_handler support parameters will be a very good feature.

https://github.com/inaka/cowboy_swagger/blob/master/src/cowboy_swagger_json_handler.erl#L31

@elbrujohalcon
Copy link
Member

Not a bad idea, but I guess it requires many more changes than the ones you just submitted, right?

@turtleDeng
Copy link
Contributor Author

This is a compatibility modification
Support the use

trails:trails([{cowboy_swagger_handler, #{server => xxx}}]) and
trails:trails([cowboy_swagger_handler])

@turtleDeng
Copy link
Contributor Author

Our project will listen to multiple http ports, and their APIs are different, trails: trails does not support parameters will cause me to be unable to open swaggerUI

@elbrujohalcon
Copy link
Member

Sorry, @turtleDeng … But you'll have to be a bit more detailed than that.
On one hand, I kinda see what you're aiming to achieve… but on the other hand your PR seems to imply that the rest of the code in this project already supports that and I have no compelling reason to think that's true.

@elbrujohalcon
Copy link
Member

Our project will listen to multiple http ports, and their APIs are different, trails: trails does not support parameters will cause me to be unable to open swaggerUI

I see… and just by changing that single line in trails_handler you make it work? That's what seems odd to me. Don't you need to change other stuff in the project to add support for it?

@elbrujohalcon
Copy link
Member

You, at the very least, need to add a new callback to trails_handler. And probably make it optional, I think.

@turtleDeng
Copy link
Contributor Author

Yes, add a trails/1 callback

@elbrujohalcon
Copy link
Member

elbrujohalcon commented Jul 26, 2021

Great! Now, can you please do the following things?

  • make the new callback optional (as a matter of fact, you'll probably like both callbacks to be optional now… so you can implement just one of them).
  • add some tests to trails_SUITE for this new feature
  • adjust the documentation for trails_handler callbacks in the README

test/trails_test2_handler.erl Outdated Show resolved Hide resolved
Copy link
Member

@elbrujohalcon elbrujohalcon left a comment

Choose a reason for hiding this comment

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

Two additional small changes and we're done.

test/trails_SUITE.erl Outdated Show resolved Hide resolved
src/trails_handler.erl Show resolved Hide resolved
@elbrujohalcon
Copy link
Member

Well… now we have some dialyzer issue in our hands… 🤔

@elbrujohalcon elbrujohalcon merged commit 0cd0385 into inaka:master Jul 27, 2021
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.

None yet

2 participants