Skip to content

Conversation

@madsodgaard
Copy link
Contributor

@madsodgaard madsodgaard commented Oct 31, 2025

Correctly propagates any configuration from swift-java.config to the command line tool, when using the build plugin.

Also fixes the docs to correctly specify the values of enum options, as they use camelCase and not kebab-case.

Copy link
Collaborator

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Thanks for the fix but we should approach this differently,

make all CLI options optional and only "override" the config values if they were set. This way we don't have to go to the plugin and pass strings anywhere, and it more models what we actually want:

func configure<T>(_ setting: inout T, overrideWith value: T?) {
  if let value {
    setting = value
  }
}

and

    config.outputJavaDirectory = outputJava
    config.outputSwiftDirectory = outputSwift
    configure(&config.unsignedNumbersMode, overrideWith: unsignedNumbers) <<<<
    config.minimumInputAccessLevelMode = minimumInputAccessLevel
    config.memoryManagementMode = memoryManagementMode

The problem is @Flag which does not support Bool? huh... so we need to figure that one out 🤔 Perhaps by manually parsing the command line arguments to see if it was passed or not?

@madsodgaard madsodgaard requested a review from ktoso October 31, 2025 11:40
Copy link
Collaborator

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

LGTM thanks for adjusting how we do this :)

@ktoso
Copy link
Collaborator

ktoso commented Nov 4, 2025

We're having CI issues... I'll see what I can do about that, sadly 6.2 didn't get the fix yet we're waiting on

@ktoso ktoso merged commit c58bbed into swiftlang:main Nov 4, 2025
67 of 78 checks passed
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