-
Notifications
You must be signed in to change notification settings - Fork 324
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
Create execution context with id #6947
Conversation
@@ -707,7 +707,9 @@ impl model::project::API for Project { | |||
) -> BoxFuture<FallibleResult<model::ExecutionContext>> { | |||
async move { | |||
let ls_rpc = self.language_server_rpc.clone_ref(); | |||
let context = execution_context::Synchronized::create(ls_rpc, root_definition); | |||
let context_id = Uuid::new_v4(); |
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.
Random UUID is created on the IDE side and send to the LS+engine to use it. That's a change to current behavior when no UUID is send and the LS+engine thinks one up. As a result the randomness is on the IDE side and the engine behaves (more) deterministically when fed with the same stream of messages.
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 is slightly related to #6718
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.
Please change the UUID generation place and it's good to go.
pub async fn new( | ||
project: model::Project, | ||
method: MethodPointer, | ||
context_id: model::execution_context::Id, | ||
) -> FallibleResult<Self> { | ||
let graph = controller::Graph::new_method(&project, &method).await?; | ||
let execution = project.create_execution_context(method.clone()).await?; | ||
let execution = project.create_execution_context(method.clone(), context_id).await?; |
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 would generate context ID here instead of https://github.com/enso-org/enso/pull/6947/files#diff-43cfb2975b079bdc838c391f870ff5150a1eb5807e912f5d1230432b76a5fc46R134
This is graph controller, and controller should have API "for the view". The view have no interest in generating context IDs.
Pull Request Description
executionContext/create
method has an optionalcontext_id
parameter. Supplying this argument makes the user's session more reproducible. I.e. this way the language server can recreate the user's session by recording the requests.Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.