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

Add support for labeled parameters in *ArgValues array #283

Open
wvteijlingen-npo opened this issue Jan 31, 2025 · 5 comments
Open

Add support for labeled parameters in *ArgValues array #283

wvteijlingen-npo opened this issue Jan 31, 2025 · 5 comments

Comments

@wvteijlingen-npo
Copy link

wvteijlingen-npo commented Jan 31, 2025

Given the following protocol:

protocol MyProtocol {
    func myMethod(foo: Int, bar: String, baz: String)
}

The current way to check arguments is:

#expect(instance.myMethodArgValues.first?.0 == 1)
#expect(instance.myMethodArgValues.first?.1 == "something")
#expect(instance.myMethodArgValues.first?.2 == "something else")

While this works perfectly, it is not very readable. The arguments are stored as a tuple, and when reading this code it is not immediately clear which value of the tuple corresponds to which method parameter. You need to manually map each index to each parameter.

In contrast, the following is much easier to immediately understand without knowing the signature of myMethod:

#expect(instance.myMethodArgValues.first?.foo == 1)
#expect(instance.myMethodArgValues.first?.bar == "something")
#expect(instance.myMethodArgValues.first?.baz == "something else")

Proposal

Currently all the arguments are stored in a tuple without labels. My proposal is to store all arguments in either a tuple with labels, or a struct with separate properties.

Preventing breaking changes

Restructuring the myMethodArgValues property like this would break all current code, which is undesirable.

To mitigate this we can either:

  1. Provide a separate property, for example: myMethodLabeledArgValues.
var myMethodArgValues = [(Int, String, String)]()
var myMethodLabeledArgValues = [(foo: Int, bar: String, baz: String)]()
  1. Use both unlabeled as well as labeled elements in the current tuple:
var myMethodArgValues = [(Int, String, String, foo: Int, bar: String, baz: String)]()

Conclusion

I think such a change would tremendously improve the experience with this package. Would love to hear some thoughts about this.

@wvteijlingen-npo wvteijlingen-npo changed the title Add support for named parameters in *ArgValues array Add support for labeled parameters in *ArgValues array Jan 31, 2025
@etiennemartin
Copy link

etiennemartin commented Feb 7, 2025

I'm really happy to see this Issue. I was looking to do the same thing since it's the biggest point of friction in adopting this mocking library. We are currently looking to move away from SwiftyMocky since it died. I was trying to see why this wasn't done before hand. My only assumption was that it would hinder performance. Is that the case?

Preventing breaking changes

If I may, I would suggest turning off this feature if we are worried about backwards compatibility and creating a new command line argument to enable it. By default it can leave the existing behaviour, and enabling it could allow for both to be generated, or just the named ones? Maybe something like --enable-named-args-history

Thoughts?

@wvteijlingen-npo
Copy link
Author

Using a command line argument to switch between numeric and labeled tuples seems like a neat solution.

I believe there should be an option to support both styles, allowing for gradual adoption. Otherwise, developers would need to update their entire test suite all at once, which would likely be impractical for larger projects.

@sidepelican
Copy link
Collaborator

I believe that backward compatibility can be maintained without the option. Are there any potential issues you see?

let t = (a: 0, b: 1, 2)
t.0 // 0
t.a // 0
t.2 // 2

@etiennemartin
Copy link

Are there any potential issues you see?

Good point, do we have any concerns over performance? If we can enable this by default without any backward compatibility or performance issues, I'm all for it!

@maxim-chipeev
Copy link

+1 on this, this would be great for readability and reduce friction for adopting this library.

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

No branches or pull requests

4 participants