-
Notifications
You must be signed in to change notification settings - Fork 997
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
[Navigation] Allow case-insensitive matches of Enum args in deep links #152
[Navigation] Allow case-insensitive matches of Enum args in deep links #152
Conversation
return constant | ||
} | ||
if (enumConstant.name.equals(value, ignoreCase = true)) { | ||
caseInsensitiveMatch = constant | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am missing why we need to break here instead of returning the match?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. We don't need to break here. But it probably shouldn't return straight away either. I would think it should continue looping the remaining enum values for an exact match and return that if found, and only return the caseInsensitiveMatch
if it doesn't end up finding an exact match.
If you have a deeplink url of https://myapp.com/someenumvalue
and an enum:
enum class MyEnum {
SomeEnumValue
someenumvalue
}
We would not want the case insensitive match on the frst item to be returned when the 2nd item will be an exact match.
I've updated the PR to do that. If that's still not right let me know and I can make further updates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to match to that level. We specifically want the Enums to be case insensitive here, so distinguishing by case after that match may not be reasonable.
I would argue that we do want the first item to be returned to discourage this type of confusing distinction. If we look for the exact match, it would allow the following:
enum class MyEnum {
RED
REd
Red
red
(etc..)
}
Now we are back in the case where all of these could potentially point to different destinations, which is the problem with this bug in the first place. Maybe provide it and hope no one does this type of thing, but I tend to err on the side that if you can do it someone will. @ianhanniballake curious on your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. If it should just do a simple case-insensitive match, then I can probably update it to use firstOrNull()
which IMO will make it more concise/readable as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let's use firstOrNull()
here - the first case insensitive match is the one we want to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jbw0033 @ianhanniballake - Updated to do a simple case-insensitive match.
@jbw0033 - I pushed an update and the lint check fails with |
@Bradleycorn I'm not sure why this is happening yet, but it looks like it's comparing from a commit from April 2 and failing to find the sha in the checkout. Could you try rebasing your changes on top of latest on |
I figured that might be the issue. I'll try it. |
…hing values are found when doing the case-sensitive matchiing.
…on as it is found. Instead keep looking for an exact match.
@dlam Rebasing seems to have fixed it, thanks! |
uh oh. @dlam now the build is failing, claiming that the version of |
Yep feel free to just ignore that it's a known failure on HEAD atm, working on fixing this :) |
Proposed Changes
Deep Link arguments that are Enum types would only match and get parsed if the casing of the argument value was an exact match to the casing of the Enum value. Since enum values are typically all upper case, this required deep links to contain upper case values as well, meaning if you have an enum with a value of say
Bitmap.Config.ALPHA_8
, a deep link that has an argument of typeBitmap.Config
will only match if the URL contains/ALPHA_8
and will not match a url with/alpha_8
.This PR updates the EnumType value parsing to include case-insensitive matching. It will first try to parse an enum value using a case-sensitive match, and if no match is found, it will then try to parse the enum value using a case-insensitive match.
Given the following enum:
And a destination with an Enum argument and deep link:
This PR updates the EnumType parsing so that a url like
https://myapp.com/green
will be matched and thedestColor
will be set toColors.GREEN
Testing
NavTypeTest
to testEnumType.parseValue()
asserting that both case-sensitive and case-insensitive values are matched.Test: /gradlew test connnectedCheck
Issues Fixed
https://issuetracker.google.com/issues/135857840
Fixes: b/135857840