-
Notifications
You must be signed in to change notification settings - Fork 5
Add new playground types #21
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
Conversation
Needed to get playground support into SourceKit-LSP: swiftlang/sourcekit-lsp#2340
| /// This property is always present whether the `Playground` has a `label` or not. | ||
| /// | ||
| /// Follows the format output by `swift play --list`. | ||
| public var id: String |
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.
I think this needs to capture either the corresponding product or target as well so that the playground can be executed in the appropriate context
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.
@owenv the LSP code that parses these does include the target https://github.com/swiftlang/sourcekit-lsp/pull/2340/files#diff-d76b1dc09dd52af2a88684043d44d1e5261f013c09f6137dffd4e23aebce6e56R91 but do you want the comment code to reflect the expected format?
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.
I added a better explanation about id but let me know if there is more you wanted
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.
Hmm, so it seems like the playgrounds implementation is building a dylib composed of every target in the package and linking the runner against that. This will work in small examples, but in general it's not safe to assume all the targets in a package can be safely linked into a single image. e.g. they may have different platform requirements, conflicting static initializers, multiple copies of a binary dependency built from the same sources, etc. I think sourcekit-lsp will need to pick a single specific appropriate library product containing the playground's code, thread that through to the play command, and adjust the build of the runner accordingly
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.
Whatever swift play --list returns is what I think we should aim to match, and then VSCode executing swift play myPlaygroundID should make sure the right one is built and run
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.
Any further comments @owenv? Can we merge and tag a new release?
ahoppen
left a comment
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.
A couple small comments, otherwise LGTM.
Sources/LanguageServerProtocol/SupportTypes/TextDocumentPlayground.swift
Outdated
Show resolved
Hide resolved
Co-authored-by: Alex Hoppen <alex@alexhoppen.de>
…ound.swift Co-authored-by: Alex Hoppen <alex@alexhoppen.de>
Needed to get playground support into SourceKit-LSP: swiftlang/sourcekit-lsp#2340