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

Move mapping rule checking to MappingRule and add missing tests #573

Merged
merged 2 commits into from
Feb 5, 2018

Conversation

davidor
Copy link
Contributor

@davidor davidor commented Feb 5, 2018

Refactor.
This PR moves the method that checks a mapping rule againts a request from the Service module to the MappingRule module. I think it clearly belongs there.
This PR also adds some unit tests for the matching of mapping rules.

-- @tparam string path Path of an HTTP request.
-- @tparam table args Table with the args and values of an HTTP request.
-- @treturn boolean Whether the mapping rule matches the given request.
function _M:matches(method, path, args)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we introduce SOAP rules that are full URL the path will no longer be just path, but a full URI. But a pathj is an URI. So maybe lets call it uri ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed 👍

Notice that this commit does not change functionality. It just moves a
method.
While at it, remove and old FIXME in Service that references params
that no longer exist.
@davidor davidor force-pushed the move-mapping-rule-check branch from 4c2c0b9 to 8e2d980 Compare February 5, 2018 11:22
@davidor davidor merged commit 323fd40 into master Feb 5, 2018
@davidor davidor deleted the move-mapping-rule-check branch February 5, 2018 11:51
@davidor davidor mentioned this pull request Feb 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants