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

Some named matches fixes #130

Merged
merged 11 commits into from
Sep 10, 2018

Conversation

ilyapuchka
Copy link
Collaborator

I noticed few issues with named matches, i.e. it was not possible to have multiple groups or mix then with unnamed. Now it's fixed and README has more details and is more accurate.

README.md Outdated
@@ -125,6 +125,8 @@ and the same test will produce logs:
step User is logged in as a registered user
```

In step implementation you will access matched values using the name of the group, i.e. `match["aRegisteredUser"]`. You can access all matched values (including matched by unnamed groups) by their index, starting from 1, i.e. `match["1"]` (note that key is still a string). So you can have more than one named group and you can mix them with unnamed groups.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be nice (in a later pull request) for this to not be a dictionary, but by a custom type which allowed strings as named matches, and integers to get unnamed ones?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can do it with this PR, I already played with this idea, but didn't finish it. I'll also remove then variants of steps with dictionaries as they create unnecessary variations of API

@@ -8,24 +8,41 @@

import Foundation

protocol StepFunctionParameters {}
public struct StepMatches<T: MatchedStringRepresentable> {
public let allMatches: [T]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be public if we are controlling access via the subscript methods (imho they shouldn't be, I'd make them private :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we can expose one or another but IMO there is no point in private subscripts, so I'll make properties private and leave subscripts public, they are also nices from user point of view

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I didn't explain myself clearly - that's exactly what I thought, private properties and public subscripts :)

@deanWombourne
Copy link
Contributor

Hey - really like StepMatches :)

@ilyapuchka ilyapuchka merged commit 4d25466 into net-a-porter-mobile:master Sep 10, 2018
@ilyapuchka ilyapuchka deleted the named-matches branch September 10, 2018 17:52
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

2 participants