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

feat(ios-swift): adding extension formatter #724

Conversation

antoniogamizbadger
Copy link
Contributor

@antoniogamizbadger antoniogamizbadger commented Oct 25, 2021

Issue #, if available: #719

Description of changes:

I have added a formatter to swift to allow the use of extension. For instance:

//
// StyleDictionarySize.swift
//

// Do not edit directly
// Generated on Mon, 25 Oct 2021 13:31:16 GMT

import UIKit
import SwiftUI

 extension CGFloat {
    static let bodyM = CGFloat(224.00)
    static let bodyMini = CGFloat(144.00)
    static let bodyS = CGFloat(192.00)
    static let bodyXmini = CGFloat(128.00)
    static let bodyXs = CGFloat(160.00)
    static let headingL = CGFloat(256.00)
    static let headingXl = CGFloat(320.00)
    static let title = CGFloat(320.00)
}

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Member

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

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

This is a great start! Thanks for taking this work on. Just 2 minor comments. If you can, it would be good to include some integration tests so we can verify the output. You can use these integration tests as inspiration: https://github.com/amzn/style-dictionary/blob/main/__integration__/iOSObjectiveC.test.js

lib/common/templates/ios-swift/extension.swift.template Outdated Show resolved Hide resolved
<%= fileHeader({file, commentStyle: 'short'}) %>
import UIKit

public enum <%= file.className %> {
Copy link
Member

Choose a reason for hiding this comment

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

Are all extensions enums? Or does that need to be configurable too?

Copy link
Contributor

@gabischima gabischima Nov 9, 2021

Choose a reason for hiding this comment

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

sorry for my intrusion (: i came here to check what's already in progress that could meet my needs
but could this be achieved by passing a parameter for prefix and type?
prefix would be access level say public or internal, and type if enum, class, struct or extension.
i have a use case where i need my tokens to be internal access where only my SPM has access to it.

if no prefix is passed could be public by default.

Suggested change
public enum <%= file.className %> {
<%= file. prefix ? 'public' : file.prefix %> <%= file.type %> <%= file.className %> {

[EDIT]
i understand if it's too specific use case (: is just an idea for maybe not having to create multiple templates for each type class/enum/extension.
[/EDIT]

Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like a good idea. Thanks @gabischima

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the case of extension the className has to be the name of the type you are extending. Right now this parameter has to be provided by the user manually, is there a problem with that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All extensions are not enums, that was my mistake.

Copy link
Contributor Author

@antoniogamizbadger antoniogamizbadger Nov 12, 2021

Choose a reason for hiding this comment

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

Just a small doubt, how would we unify the three templates into one while keeping backward compatibility? I mean, with a template for every type we can do something like this:

  {
    "destination": "StyleDictionarySize.swift",
    "format": "ios-swift/extension.swift",
    "className": "CGFloat",
    "filter": {
      "attributes": {
        "category": "size"
      }
    }
  }

Without specifying any type option. If we only use one template, I think we have two options:

  • Option 1: use the configuration above adding type and specifying the formatter on format.
  • Option 2: only specify format and override type option in format.js. Something like:
  'ios-swift/extension.swift': function({dictionary, options, file}) {
    const template = _template(
      fs.readFileSync(__dirname + '/templates/ios-swift/swift.template')
    );
    let allTokens;
    const { outputReferences } = options;
    file['type'] = 'extension';
    const formatProperty = createPropertyFormatter({
      outputReferences,
      dictionary,
      formatting: {
        type: 'extension',
        suffix: ''
      }
    });

And the same change would be done for class and enum.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These changes (and your intuition) are spot on. I think @dbanksdesign has a couple comments, but beyond that I think we have a solid PR here. Thank you!

Copy link
Member

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

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

Love the direction! Just a few minor comments

lib/common/formats.js Outdated Show resolved Hide resolved
lib/common/formats.js Outdated Show resolved Hide resolved
@@ -18,9 +18,10 @@
//
<%= fileHeader({file, commentStyle: 'short'}) %>
import UIKit
import SwiftUI
Copy link
Member

Choose a reason for hiding this comment

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

Are there any issue with including both UIKit and SwitfUI? I would assume most people use one or the other? Should we make this a configuration as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, there's no additional drawback to have them both imported. We could make it a part of the configuration, but the only benefit would to have one line less of code. Import them both also does not affect app size because they are both available in every iOS environment anyway.

Copy link

Choose a reason for hiding this comment

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

The drawback would be that this could potentially be used on macos which does not have UIKit! But it does have SwiftUI. So to be more Apple Platform agnostic it would have been better to leave this out. I have been struggling with just that now.

@antoniogamizbadger antoniogamizbadger marked this pull request as ready for review December 13, 2021 13:11
@chazzmoney
Copy link
Collaborator

We merged #734 instead of this one, but credited you in the commit message and the release notes. Thank you for your contribution - it was an important need and very much appreciated!

@chazzmoney chazzmoney closed this Jan 4, 2022
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

5 participants