Skip to content

Fixed JSON parsing error while selecting toolchain #1743

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 4 commits into from
Jul 25, 2025

Conversation

roulpriya
Copy link
Contributor

@roulpriya roulpriya commented Jul 23, 2025

Description

In the swiftly toolchain version schema, there was a missing name attribute, which prevented it from parsing the value. In this fix, I have removed the name field from the top-level and added it at the object level.

Issue: #1739

Tasks

  • Required tests have been written
  • Documentation has been updated
  • Added an entry to CHANGELOG.md if applicable

@roulpriya roulpriya marked this pull request as ready for review July 23, 2025 15:15
@award999
Copy link
Contributor

Was the issue that there was a missing "name" property? If so would that mean if swiftly changed the schema in the future then the extension could fail again? Can you maybe update the PR description to explain what this issue was and why this fixes it

@roulpriya
Copy link
Contributor Author

Was the issue that there was a missing "name" property? If so would that mean if swiftly changed the schema in the future then the extension could fail again? Can you maybe update the PR description to explain what this issue was and why this fixes it

I don't expect things to change once we release Swiftly with JSON support. Swiftly will ensure backwards compatibility with the JSON format. Newer formats will always be opt-in

@matthewbastien
Copy link
Member

I don't expect things to change once we release Swiftly with JSON support. Swiftly will ensure backwards compatibility with the JSON format. Newer formats will always be opt-in

I guess my bigger question would be: can we only check for the fields that we actually care about and ignore the rest? E.g. the name field can just be omitted if we aren't using it at all.

I can imagine Swiftly might add new information to the output later on. This wouldn't be considered a breaking change from Swiftly's perspective, but would cause the current code to throw an error. (As long as I'm understanding this right)

@award999
Copy link
Contributor

that is a better worded version of my question, thanks @matthewbastien

Copy link
Member

@matthewbastien matthewbastien left a comment

Choose a reason for hiding this comment

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

Looking great! Just one more bug I found when testing the latest version of this PR on my machine.

suggestion: I don't think we need the description anymore for Swiftly toolchains in the toolchain selection dialog. It ends up just showing the same info twice now. We also don't need the toolchainPath or the swiftFolderPath since we're looking that up after the selection is made now. Both of these are going to take some amount of refactoring that could be done in a future PR, though. Up to you.

@roulpriya roulpriya force-pushed the fix-json-parsing-error branch from 7b71b1c to ce6c83f Compare July 25, 2025 18:02
Copy link
Member

@matthewbastien matthewbastien left a comment

Choose a reason for hiding this comment

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

Thank you so much for getting this ready!

@matthewbastien matthewbastien merged commit d2dc6df into swiftlang:main Jul 25, 2025
18 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.

3 participants