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

remove reverend in favor of path-to-regexp.compile #23

Merged
merged 2 commits into from
May 12, 2015
Merged

remove reverend in favor of path-to-regexp.compile #23

merged 2 commits into from
May 12, 2015

Conversation

jasisk
Copy link
Contributor

@jasisk jasisk commented May 12, 2015

Path-to-regexp, which powers Reverend, has recently added the ability to reverse routes, rendering Reverend unnecessary. As such, Reverend is in the process of being deprecated.

The sole differences between the implementation in Reverend and that of path-to-regexp is a bit of duck-typing to prevent throws. This small amount of duck-typing can be seen in the shim branch.

I've duplicated the logic from Reverend here however, if it is unnecessary, it can be removed to simplify the code.

@yahoocla
Copy link

CLA is valid!

@Vijar Vijar added the ready label May 12, 2015
@jasisk
Copy link
Contributor Author

jasisk commented May 12, 2015

Adding a test for coverage.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 8a831b0 on jasisk:remove-reverend into 9090dd3 on yahoo:master.

@mridgway
Copy link
Collaborator

Wow, thanks so much for updating your dependents! We'll get this merged in ASAP.

@lingyan Want to take a look?

@jasisk
Copy link
Contributor Author

jasisk commented May 12, 2015

Happy to.

As mentioned, I codified the existing behavior but it's just a carryover from a legacy path-to-regexp. I don't think it necessarily makes sense but I wanted to capture it and leave that decision up to you fine folks. 😀

@jasisk
Copy link
Contributor Author

jasisk commented May 12, 2015

One difference worth mentioning:
path-to-regexp accounts for trailing slash cases that we never did (though we had an issue filed against it).
The impacted cases can be seen in the tests from the shim branch.

@mridgway
Copy link
Collaborator

Absolutely, makes sense. We'll take it from here and decide which parts we need or don't need.

@lingyan
Copy link
Member

lingyan commented May 12, 2015

@jasisk This is awesome. Thanks a lot for the PR. There is some minor changes we can make. I will merge the PR and we can take it from here. Thanks again!

lingyan added a commit that referenced this pull request May 12, 2015
remove reverend in favor of path-to-regexp.compile
@lingyan lingyan merged commit 97d14ff into yahoo:master May 12, 2015
@lingyan lingyan removed the ready label May 12, 2015
@jasisk jasisk deleted the remove-reverend branch May 12, 2015 21:15
@lingyan
Copy link
Member

lingyan commented May 14, 2015

@jasisk FYI, new patch version released with your contribution: https://github.com/yahoo/routr/releases/tag/v0.1.2 Thanks!

@jasisk
Copy link
Contributor Author

jasisk commented May 14, 2015

My pleasure. It's always frustrating when something goes away quietly. Figured I'd help alleviate some of that frustration. Plus you all do excellent work—happy to make a minuscule contribution to it.

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

6 participants