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

Feature: Swift argument parser #46

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jsanderson44
Copy link
Contributor

Removed the old CommandLineKit dependency in favour of the Swift Argument Parser package (see https://swift.org/blog/argument-parser/). This change IS backwards compatible.

@jsanderson44 jsanderson44 requested review from Neil3079 and samdods July 22, 2020 16:30
@jsanderson44 jsanderson44 self-assigned this Jul 22, 2020
Base automatically changed from chore-rebrand to master July 23, 2020 09:11
Copy link

@bengilroy bengilroy left a comment

Choose a reason for hiding this comment

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

Looks good! A couple of things:

  • As we discussed on Slack it seems well have to use Xcode 11.4 and above to contribute to Configen from now on, perhaps we should add something to the readme to point this out?
  • I'd maybe update the configen binary in the ConfigenDemo app to the new one (I did this to test it and it shows as modified on git so think it needs updating anyway).


var autoGenerationComment: String {
return """
// auto-generated by \(optionsParser.appName)\n// to add or remove properties, edit the mapping file: '\(optionsParser.inputHintsFilePath)'.\n// README: https://github.com/theappbusiness/ConfigGenerator/blob/master/README.md\n\n
// auto-generated by \(options.appName)\n// to add or remove properties, edit the mapping file: '\(options.inputHintsFilePath)'.\n// README: https://github.com/theappbusiness/ConfigGenerator/blob/master/README.md\n\n
"""

Choose a reason for hiding this comment

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

Unrelated, but if we're using a multi-line string we could just use return as normal instead of manually entering \n – unless it's there for another reason.

There's also a few other normal strings that are using \n so for readability we could consider making those multiline too, if we feel it does make it more readable that is. Could be something for a future PR anyway.

@MarcoGuerrieriTAB MarcoGuerrieriTAB changed the base branch from master to main September 15, 2020 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants