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

Adds swift FML code generation #4780

Merged
merged 17 commits into from
Jan 28, 2022
Merged

Adds swift FML code generation #4780

merged 17 commits into from
Jan 28, 2022

Conversation

tarikeshaq
Copy link
Contributor

@tarikeshaq tarikeshaq commented Jan 17, 2022

fixes EXP-1984

  • Allows running swift scripts against FML generated code
  • Allows running swift script against a mock nimbus feature variables API
  • Generate swift FML code similar to how Kotlin does it
  • Write tests to cover at minimum what the Kotlin tests cover
  • Clean up FeatureVariables API so UIKit is imported again
  • Have the "real" nimbus API use `FeatureInterface"
  • Clean up code and TODOs in comments
  • Make unit tests pass
  • Rebase
  • Test against Firefox iOS to make sure there are not breaking changes
  • Changelog

@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2022

Codecov Report

Merging #4780 (fb40792) into main (134c50e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4780   +/-   ##
=======================================
  Coverage   81.49%   81.49%           
=======================================
  Files          49       49           
  Lines        5653     5653           
=======================================
  Hits         4607     4607           
  Misses       1046     1046           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 134c50e...fb40792. Read the comment docs.

Copy link
Contributor Author

@tarikeshaq tarikeshaq left a comment

Choose a reason for hiding this comment

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

Early notes, I think I have the testability of this setup nicely. Scripts can run and access the MockNimbus API. I have to think a little about the UIKit dependency, but I can do that later when this PR is in a ready state 🤷

Copy link
Contributor

@jhugman jhugman left a comment

Choose a reason for hiding this comment

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

Small 🚲 🚗 🚜 🚌 Drive By review.

The Rust looks boring and regular; good job; otherwise, I haven't commented much.

I've added a few comments about the Swift. Keep going looks nice.

Copy link
Member

@travis79 travis79 left a comment

Choose a reason for hiding this comment

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

Drive-by review (I just read James' review and I didn't have much to add beyond his suggestions to make the code a little more "swifty").

components/nimbus/ios/Nimbus/FeatureVariables.swift Outdated Show resolved Hide resolved
components/nimbus/ios/Nimbus/FeatureVariables.swift Outdated Show resolved Hide resolved
@tarikeshaq tarikeshaq marked this pull request as ready for review January 24, 2022 02:03
@tarikeshaq tarikeshaq changed the title [WIP] Adds swift FML code generation Adds swift FML code generation Jan 24, 2022
@tarikeshaq
Copy link
Contributor Author

OK tested against firefox-ios locally and verified there are no breaking changes for Nimbus (had to test against the places_uniffication and the glean update patches since those are on main here, but regardless, this patch doesn't introduce a breaking change)

also added the changelog so this should be in a good place for review 😄

Copy link
Member

@travis79 travis79 left a comment

Choose a reason for hiding this comment

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

Just a handful of minor nits, but this looks fantastic! Well done!

CHANGES_UNRELEASED.md Show resolved Hide resolved
components/nimbus/ios/Nimbus/Nimbus.swift Outdated Show resolved Hide resolved
components/support/nimbus-fml/fixtures/fe/ios.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@jhugman jhugman left a comment

Choose a reason for hiding this comment

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

Lovely.

I've left some comments which should be addressed, but nothing to hold up another review cycle.

components/nimbus/ios/Nimbus/FeatureInterface.swift Outdated Show resolved Hide resolved
Comment on lines 12 to 15
if let s = s {
return {{class_name}}(rawValue: s)
}
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

if let is a nil check, so it's redundant here.

Suggested change
if let s = s {
return {{class_name}}(rawValue: s)
}
return nil
return {{class_name}}(rawValue: s)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the nil check is intentional here because the enumValue in swift takes a String?, since the v.getString("the-property") returns a String?

return (k1, v1)
}
return [K1: V1](uniqueKeysWithValues: transformed)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about these kotlin names; in general I've tried to keep naming schemes familiar to the platform, but this is all very internal.

@@ -0,0 +1,11 @@
import FeatureManifest
Copy link
Contributor

Choose a reason for hiding this comment

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

This is … suprising. I'm not sure I understand why this is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only for the tests, the reason is that the scripts are run after the FeatureManifest, and all the Nimbus helpers are compiled together into a module that's later imported by the script. In the app, the FeatureManifest would be compiled together with the app so this wouldn't be needed

@tarikeshaq tarikeshaq merged commit 42af47f into main Jan 28, 2022
@tarikeshaq tarikeshaq deleted the swift-fml branch January 28, 2022 02:38
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.

4 participants