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

Make url scheme and host match and register case insensitive #222

Merged
merged 3 commits into from
Oct 22, 2020

Conversation

JOCuendet
Copy link
Contributor

Checklist

Motivation and Context

The current implementation did not account for RFC-3986, and by doing so if the user entered an URL with a capital letters in the scheme or the host the match would fail.
By complying to the norm, this behaviour is now accounted for, and the issue is fixed.

Description

Forcing the scheme and host from the user's input URL to be lowercased before parsing them to the match method.
Changed a UnitTest to accommodate to the base code changes.
Tested applying the code to a working project and running the app on a physical device to debug the deep links and universal links

@JOCuendet JOCuendet changed the title add rfc3986-compliance Add case insensitive to URL by complying to RFC 3986 Oct 21, 2020
@JOCuendet JOCuendet changed the title Add case insensitive to URL by complying to RFC 3986 Add case insensitive to deeplinks by complying to RFC 3986 Oct 21, 2020
@JOCuendet JOCuendet force-pushed the feature/add-rfc3986-compliance branch from cef2d5b to 7bf4433 Compare October 21, 2020 16:36
@JOCuendet JOCuendet changed the title Add case insensitive to deeplinks by complying to RFC 3986 Add case insensitive to deeplinks Oct 21, 2020
Copy link
Collaborator

@filipe-lemos filipe-lemos left a comment

Choose a reason for hiding this comment

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

Nice fix! 🎉 Perhaps add a couple of unit tests to Route+TrieRouter_RouteTests that check for case-insensitive matches.

@codecov
Copy link

codecov bot commented Oct 21, 2020

Codecov Report

Merging #222 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #222   +/-   ##
=======================================
  Coverage   94.84%   94.84%           
=======================================
  Files          97       97           
  Lines        3198     3200    +2     
=======================================
+ Hits         3033     3035    +2     
  Misses        165      165           
Impacted Files Coverage Δ
Sources/DeepLinking/Route+TrieRouter.swift 91.78% <100.00%> (+0.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8e6f53...8b98c36. Read the comment docs.

Copy link
Member

@p4checo p4checo left a comment

Choose a reason for hiding this comment

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

Nice improvement! 💪

Some suggestions:

  • please add some new unit tests to check new behavior 🤓
  • the PR title (and final commit) could still be improved: perhaps "Make route scheme and host checks case insensitive"? 💅

Sources/DeepLinking/Route+TrieRouter.swift Show resolved Hide resolved
@JOCuendet JOCuendet changed the title Add case insensitive to deeplinks Make url scheme and host match and register case insensitive Oct 22, 2020
@JOCuendet JOCuendet changed the title Make url scheme and host match and register case insensitive Make url scheme and host, match and register case insensitive Oct 22, 2020
@JOCuendet JOCuendet changed the title Make url scheme and host, match and register case insensitive Make url scheme and host match and register case insensitive Oct 22, 2020
Copy link
Member

@p4checo p4checo left a comment

Choose a reason for hiding this comment

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

Nice one! Just some minor tweaks left IMO 👍

@JOCuendet JOCuendet merged commit 76e7b33 into master Oct 22, 2020
@JOCuendet JOCuendet deleted the feature/add-rfc3986-compliance branch October 22, 2020 16:45
JOCuendet added a commit that referenced this pull request Oct 23, 2020
Currently the mapping and register of url routes are case sensitive
The RFC-3986 states that theses routes should be case insensitive

- Add case insensitive url routes mapping and register logic
- Add UnitTests to test, said logic
- Conform to RFC-3986
nbarreto6 pushed a commit that referenced this pull request Oct 30, 2020
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.

9 participants