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

[Fastlane.Swift] Fix OptionalConfigValue for Any-based types. #18664

Merged
merged 5 commits into from
May 19, 2021

Conversation

minuscorp
Copy link
Collaborator

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

OptionalConfigValue does not provide an ExpressibleBy interface for Any? parameters, so they have to be filtered out afterwards.
Fixes #18654

Description

OptionalConfigItem does not apply to Any types to avoid a breaking compatibility against past releases. This PR resolves the issue.

Testing Steps

  1. Install the following branch:
gem 'fastlane', :branch => 'https://github.com/minuscorp/fastlane/fix-configvalue-with-any-type'
  1. Use an action with an Any value:
incrementBuildNumber(buildNumber: <#T##Any?#>)
  1. Check that the value is being passed when it is not nil.
  2. 🚀

Copy link
Contributor

@ainame ainame left a comment

Choose a reason for hiding this comment

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

Had a minor suggestion to make it simpler but LGTM

@@ -325,7 +326,7 @@ def implementation
implm += "let args: [RubyCommand.Argument] = []\n"
else
implm += "let args = [#{args.group_by { |h| h[:name] }.keys.join(",\n")}]\n"
implm += ".compactMap { $0 }\n"
implm += ".compactMap { $0?.nilify(when: { $0.value == nil }) }\n"
Copy link
Contributor

@ainame ainame May 10, 2021

Choose a reason for hiding this comment

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

Suggested change
implm += ".compactMap { $0?.nilify(when: { $0.value == nil }) }\n"
implm += ".filter { $0?.value != nil }\n"

What about this? Isn't this simpler?

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'm an idiot

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wasn't exactly doing that, but gave me the idea to get rid of the nilify method ❤️

Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

I tried doing incrementBuildNumber(buildNumber: 5) but I got 👇

value of protocol type 'Any' cannot conform to 'ExpressibleByIntegerLiteral'; only struct/enum/class types can conform to protocols

Am I doing something wrong? 😇

@minuscorp
Copy link
Collaborator Author

minuscorp commented May 14, 2021

But it works fine with you for float and string values?🤔

EDIT: I fixed it I think (tested as much as I could)

@minuscorp minuscorp requested a review from joshdholtz May 14, 2021 10:03
@RyuX51
Copy link

RyuX51 commented May 14, 2021

I tried the fix-configvalue-with-any-types branch and I'm getting

▸ Compiling Fastfile.swift

❌  .../Fastfile.swift:22:21: cannot convert value of type 'String' to expected argument type 'OptionalConfigValue<String?>'

      echo(message: "Unknown parameter value \(bumpType)")
                    ^


** BUILD FAILED **

for the following code:

  func betaLane(withOptions options: [String: String]?) {

    let bumpType = options?["bumpType"] ?? "patch"

    if !["major", "minor", "patch"].contains(bumpType) {
      echo(message: "Unknown parameter value \(bumpType)")
      return
    }

If I do without string interpolation it works fine. Thanks.

Since I'm coming from 18654, I'll just drop this here.

@minuscorp
Copy link
Collaborator Author

@RyuX51 could you check that your issue is solved now?

@RyuX51
Copy link

RyuX51 commented May 14, 2021

@minuscorp I'm sorry, c1b9cf1still behaves the same.

@minuscorp
Copy link
Collaborator Author

bumpType is String or String??

@RyuX51
Copy link

RyuX51 commented May 14, 2021

It's String, but this isn't the problem. For example

echo(message: "Unknown " + "parameter")

and

echo(message: "Unknown \("parameter")")

also lead to the same error whereas

echo(message: "Unknown parameter")

is just fine.

@minuscorp
Copy link
Collaborator Author

minuscorp commented May 14, 2021

I just tested using

let string = "Foo"

echo(message: "Message \(string)")

Working🤔

@RyuX51
Copy link

RyuX51 commented May 14, 2021

@minuscorp

Using fastlane 2.182.0 from https://github.com/minuscorp/fastlane (at fix-configvalue-with-any-types@c1b9cf1)

So this should be the right commit, right?

▸ Compiling Fastfile.swift

❌  /Users/mario/Repository/VJeton/fastlane/Fastfile.swift:24:12: missing argument label 'message:' in call

      echo("Message \(string)")
           ^



❌  /Users/mario/Repository/VJeton/fastlane/Fastfile.swift:24:12: cannot convert value of type 'String' to expected argument type 'OptionalConfigValue<String?>'

      echo("Message \(string)")
           ^


** BUILD FAILED **

And when I add the message: it's again:

▸ Compiling Fastfile.swift

❌  /Users/mario/Repository/VJeton/fastlane/Fastfile.swift:24:21: cannot convert value of type 'String' to expected argument type 'OptionalConfigValue<String?>'

      echo(message: "Message \(string)")
                    ^


** BUILD FAILED **

@minuscorp
Copy link
Collaborator Author

minuscorp commented May 14, 2021

Pretty confusing to me 😔
image

Whereas
image

Does fail because there isn't interface in Swift for that kind of Expressible afaik

@RyuX51
Copy link

RyuX51 commented May 14, 2021

@minuscorp Even echo(message: String("Message")) fails for me. 🤷‍♂️

@minuscorp
Copy link
Collaborator Author

I'd reset the repo and try to run the generate_swift_api lane before going further, maybe something in the project it's corrupted😞

@minuscorp
Copy link
Collaborator Author

@joshdholtz Can you replicate the issue or it is working fine for you?🤔

@minuscorp
Copy link
Collaborator Author

Any news @RyuX51 regarding the issue with interpolation?

@RyuX51
Copy link

RyuX51 commented May 18, 2021

@minuscorp Well, I haven't investigated further. I just set up a new project where I wanted to try a Fastlane.swift for the first time. The fact that I get this error while wanting to show a value in an informational output is not really a blocker for me. This problem just sounded a lot like what you are fixing here is related, and I just wanted to let you know. Since you can't even replicate the bug, I don't want to bother you with it any longer.

@joshdholtz
Copy link
Member

@minuscorp I’m still getting…

value of protocol type 'Any' cannot conform to 'ExpressibleByUnicodeScalarLiteral'; only struct/enum/class types can conform to protocols

when my Fastlane.swift file looks like…

incrementBuildNumber(buildNumber: "5")

Are you not getting that same thing? 🤔

@minuscorp
Copy link
Collaborator Author

@joshdholtz indeed, I removed all conformances to Optional, as it seems to not accept Any meta value, differently from AnyObject which it could conform to those extensions. Fixed and tested:

incrementBuildNumber(buildNumber: 1.0, xcodeproj: "asd")
incrementBuildNumber(buildNumber: "1.0")
incrementBuildNumber(buildNumber: 1)

Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

This works for me! ❤️ LGTM

Merge whenever you are ready

@minuscorp minuscorp merged commit e1b6a6a into fastlane:master May 19, 2021
@fastlane-bot
Copy link

Hey @minuscorp 👋

Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉
The code change now lives in the master branch, however it wasn't released to RubyGems yet.
We usually ship about once a week, and your PR will be included in the next one.

Please let us know if this change requires an immediate release by adding a comment here 👍
We'll notify you once we shipped a new release with your changes 🚀

@fastlane-bot
Copy link

Congratulations! 🎉 This was released as part of fastlane 2.184.0 🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fastlane.Swift] cannot convert value of type 'String' to expected argument type 'OptionalConfigValue<Any?>'
5 participants