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

fosite.Arguments: Nano optimization of .Exact() #399

Merged
merged 2 commits into from
Mar 3, 2020

Conversation

redbaron
Copy link
Contributor

@redbaron redbaron commented Mar 3, 2020

Previously Arguments.Exact had vague semantic where
it coudln't distinguish between value with a space and multiple
values. Split it into 2 functions with clear semantic.

Old .Exact() remains for compatibility and marked as deprecated

@CLAassistant
Copy link

CLAassistant commented Mar 3, 2020

CLA assistant check
All committers have signed the CLA.

@redbaron redbaron changed the title Nano optimization of Arguments.Exact fosite.Arguments: Nano optimization of .Exact() Mar 3, 2020
@aeneasr
Copy link
Member

aeneasr commented Mar 3, 2020

These are functionally different because before it worked for len > 1 and now it doesn’t.

@redbaron
Copy link
Contributor Author

redbaron commented Mar 3, 2020

These are functionally different because before it worked for len > 1 and now it doesn’t.

for len>1 before and after it returns false, as it should, no?

@redbaron
Copy link
Contributor Author

redbaron commented Mar 3, 2020

Only case I can see them being different is that previously .Exact() incorrectly returned true for following case:

        var v Arguments = []string{"A", "B"}
	v.Exact("A B") 

do I miss something?

@aeneasr
Copy link
Member

aeneasr commented Mar 3, 2020

        var v Arguments = []string{"A", "B"}
	v.Exact("A B") 

that is the use case I was thinking about. While it does not appear to be used like that anywhere in fosite, it was originally implemented that way because we might have arguments where order matters / must be preserved. I'm not sure if we import this code somewhere else that depends on this behavior, thus I am hesitant to merge it.

@redbaron
Copy link
Contributor Author

redbaron commented Mar 3, 2020

we might have arguments where order matters

This should be covered by func (r Arguments) ExactSlice(names []string) bool

Using .Exact(strings.Join(names," ")) for this purpose is clear misuse

@aeneasr
Copy link
Member

aeneasr commented Mar 3, 2020

That might be, but as I explained we have other code that (might) depend on this behavior and even if you have a better naming for it, it doesn't change that fact! Breaking the functionality in such a way without e.g. renaming the function as well is not something that can be merged.

If you want, you can introduce a new function ExactOne(name string) and ExactSlice and leave the existing one untouched, updating all the code where currently Exact is used and mark Exact as deprecated using godoc.

redbaron added 2 commits March 3, 2020 15:18
Previously Arguments.Exact had vague semantic where
it coudln't distinguish between value with a space and multiple
values. Split it into 2 functions with clear semantic.

Old .Exact() remains for compatibility and marked as deprecated
@redbaron
Copy link
Contributor Author

redbaron commented Mar 3, 2020

Added ExactOne and MatchesExact, also added tests for discussed edge cases

@aeneasr
Copy link
Member

aeneasr commented Mar 3, 2020

Awesome, thank you!

@aeneasr aeneasr merged commit cf23400 into ory:master Mar 3, 2020
@redbaron redbaron deleted the better-exact branch March 3, 2020 15:49
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.

3 participants