Skip to content

Refactor list command to support json format #388

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

Merged
merged 1 commit into from
Jun 20, 2025

Conversation

roulpriya
Copy link
Contributor

Fixes Issue #376
Refactor the List command to support JSON output format.

List.self, ["list", "--format", "json"], format: .json
)

let result = try JSONSerialization.jsonObject(
Copy link
Member

@cmcgee1024 cmcgee1024 Jun 18, 2025

Choose a reason for hiding this comment

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

question: Instead of decoding this JSON data into a dictionary, why not decode into a InstalledToolchainsListInfo instead? The checks below would just be simple field accesses instead of dictionary lookups.

Copy link
Contributor Author

@roulpriya roulpriya Jun 18, 2025

Choose a reason for hiding this comment

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

Doing that forces us to implement Decodable in all the structs.

Copy link
Member

Choose a reason for hiding this comment

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

Would it be much effort to make them Decodable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cmcgee1024 refactored the tests to use decode the corresponding outputdata

@roulpriya roulpriya force-pushed the refactor-list-command branch from ae57fbc to c739bf5 Compare June 20, 2025 06:22
@roulpriya roulpriya force-pushed the refactor-list-command branch from c739bf5 to c779021 Compare June 20, 2025 06:29
@cmcgee1024 cmcgee1024 merged commit 06d29fa into swiftlang:main Jun 20, 2025
24 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