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

Improve integration #1

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fedeci
Copy link

@fedeci fedeci commented Nov 27, 2021

  • Remove CodingKeys enums from FigSpec structs
  • Add option to output spec to a file
  • Add tests

public var isPersistent: Bool?
public var isRequired: Bool?
public var requiresEquals: Bool?
public var repeatCount: RepeatCount?
public var isRepeatable: RepeatCount?
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd intentionally renamed some of these properties on the Swift side to be more idiomatic -- but I do see the benefit of keeping the original names, to avoid the need for CodingKeys. As a workaround, perhaps we could (taking this specific case as an example) make isRepeatable a private property and create a public repeatCount with a computed getter and setter that simply forward to isRepeatable?

Copy link
Author

@fedeci fedeci Nov 27, 2021

Choose a reason for hiding this comment

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

Well, FigSpec should be a swift representation of the FigSpec so IMHO we should align with those property names.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm I'd say that we should be sticking to Swift's API design guidelines to keep the library idiomatic. To motivate this, consider for example a hypothetical Python library with Fig bindings: I think it'd unambiguously be better to use snake case there in order to adhere to the language's guidelines. The situation is less extreme with Swift but it's in the same spirit imo.

Sources/FigSchema/FigScriptGenerator.swift Show resolved Hide resolved
Sources/FigSchema/FigScriptGenerator.swift Show resolved Hide resolved
@@ -158,12 +162,12 @@ public struct FigSpec: Encodable {
}
}

public var names: [String]
public var arguments: [Argument]?
public var name: [String]
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether we'd benefit from an ExpressibleByArrayLiteral & ExpressibleByStringLiteral type here so that users could do either of name = "Foo" or name = ["Foo", "Bar"]

Copy link
Author

@fedeci fedeci Nov 27, 2021

Choose a reason for hiding this comment

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

Yeah it would be a nice tweak, but I am not sure that's possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

afaik it's possible; you can make a type conform to both aforementioned protocols and it should work out of the box.

Copy link
Author

Choose a reason for hiding this comment

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

I'll try it!

@fedeci
Copy link
Author

fedeci commented Nov 27, 2021

I was also thinking of making --generateFigSpec a subcommand so we can add custom options in the future (--output for example), WDYT?

@kabiroberai
Copy link
Contributor

I also thought about using a subcommand but unfortunately there are some rough edges with that approach – for example, if the root command has a body, it takes precedence over subcommands, and fixing that would require that users refactor the structure of their tool. Since our current approach relies on an OptionGroup, we might be able to add more options there itself, though I'm not 100% sure how that'd materialize.

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.

2 participants