Clean up unused code and replace Path with URL to resolve warnings #407
Conversation
belkhadir
requested review from
shahmishal,
cmcgee1024,
justice-adams-apple and
etcwilde
as code owners
| let versionContainer = try container.nestedContainer( | ||
| keyedBy: ToolchainVersionCodingKeys.self, forKey: .version | ||
| ) | ||
| let name = try versionContainer.decode(String.self, forKey: .name) |
There was a problem hiding this comment.
issue (blocking): I think that the name could be in the decoding stream, even though it is redundant for decoding because it is a synthesized property of ToolchainVersion. I expect that the name will need to be decoded here, even if the return value is thrown away. Some kind of handling of the name key here will also help with symmetry between this init and the encode method above.
There was a problem hiding this comment.
Thank you for your feedback.
To clarify, are you expecting the decode to be strict in this context?
I am considering adding:
_ = try versionContainer.decode(String.self, forKey: .name)This approach ensures that decoding fails if the name is missing, maintaining symmetry with the encode(to:) function, even if the value is not directly used.
Please let me know if this aligns with your intended solution.
|
praise: Overall this is good code cleanup. There's just one straight forward issue and then I think that this can be merged. |
| // Create generation tool arguments. | ||
| var generationToolArguments = [ | ||
| builtArtifact.path.string, | ||
| builtArtifact.url.path(), |
There was a problem hiding this comment.
Unless I’m mistaken, this should be path(percentEncoded: false).
See FB20238046 and https://developer.apple.com/forums/thread/784446.
There was a problem hiding this comment.
Nice catch! Totally makes sense since we're dealing with file system paths
| builtArtifact.url.path(), | ||
| "--output-file", | ||
| outputFile.string, | ||
| outputFile.path(), |
There was a problem hiding this comment.
Same, percentEncoded: false should be set here.
…g for subprocess
|
@belkhadir there were some problems on the main branch that were causing the workflows to fail on pull requests. You'll need to pull the latest from the main branch and incorporate them here in this PR before the workflows can succeed. Once you've done that then I'll run the workflows to perform their checks. |
|
@belkhadir thank you for your contribution. |
This merge request includes two refactoring:
PathwithURLto eliminate Xcode warnings and align with recommended API usage.