-
Notifications
You must be signed in to change notification settings - Fork 81
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
Fixed JSON parsing error while selecting toolchain #1743
Conversation
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 |
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) |
that is a better worded version of my question, thanks @matthewbastien |
There was a problem hiding this 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.
7b71b1c
to
ce6c83f
Compare
There was a problem hiding this 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!
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 thename
field from the top-level and added it at the object level.Issue: #1739
Tasks