Skip to content
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

R: After a debug session commands like n or s should be omitted from the history #2918

Open
lionel- opened this issue Apr 29, 2024 · 7 comments
Labels
area: debugger Issues related to Debugging duplicate This issue or pull request already exists enhancement New feature or request lang: r support
Milestone

Comments

@lionel-
Copy link
Contributor

lionel- commented Apr 29, 2024

Brought up in https://github.com/posit-dev/positron-beta/discussions/148.

On the one hand, as these commands are only used to advance or step in, they don't correspond to real expressions that you can use outside of debugging sessions. On the other hand, users might use the up arrow to enter a command multiple times.

RStudio takes care of both considerations and discards the commands after the debug session stops.

@lionel- lionel- added enhancement New feature or request lang: r labels Apr 29, 2024
@DavisVaughan
Copy link
Contributor

If it was easier to implement, I would not mind if we just never recorded history for n, s, c, Q, etc in the history cache

I don't think I've ever done up to re-request n 😆

@lionel-
Copy link
Contributor Author

lionel- commented Apr 29, 2024

I think the hard part is to implement custom behaviour during debug sessions (since we do need to record n etc outside of debug sessions), rather than what behaviour to implement.

@juliasilge juliasilge added this to the Future milestone Apr 29, 2024
@lionel- lionel- added the area: debugger Issues related to Debugging label May 17, 2024
@lionel-
Copy link
Contributor Author

lionel- commented Oct 7, 2024

Duplicate of #4478

@lionel- lionel- marked this as a duplicate of #4478 Oct 7, 2024
@lionel- lionel- closed this as not planned Won't fix, can't repro, duplicate, stale Oct 7, 2024
@lionel- lionel- added the duplicate This issue or pull request already exists label Oct 7, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 22, 2024
@lionel-
Copy link
Contributor Author

lionel- commented Dec 20, 2024

As pointed out by @thomasp85, not quite a duplicate!

@lionel- lionel- reopened this Dec 20, 2024
@lionel-
Copy link
Contributor Author

lionel- commented Dec 20, 2024

I would not mind if we just never recorded history for n, s, c, Q, etc in the history cache

In stats scripts there's lots of one-letter variables. I think users would wonder why they can't use Up to print a variable again.

@DavisVaughan
Copy link
Contributor

DavisVaughan commented Dec 20, 2024

When ark sends Positron a command to execute (n, c, etc), it goes through this execute() call that we control completely:

// If the DAP has commands to execute, such as "n", "f", or "Q",
// it sends events to let us do it from here.
case 'execute': {
this.execute(
msg.content.command,
uuidv4(),
positron.RuntimeCodeExecutionMode.Interactive,
positron.RuntimeErrorBehavior.Stop
);
break;
}

I wonder if we can extend this to include record_in_history: false that we can set here? That seems like it might solve this part well

@DavisVaughan
Copy link
Contributor

Oh interesting, the execute() command already kind of handles this internally. By setting positron.RuntimeCodeExecutionMode.Interactive we are requesting that store_history = true on the server side.

store_history: mode === positron.RuntimeCodeExecutionMode.Interactive,

But even if we set RuntimeCodeExecutionMode.Transient I don't think that would help. It would help the server not store the debug command in its history, but the client (Positron) isn't really respecting that in any way.

From what I can tell, Positron waits to receive an IOPub::ExecuteInput response back from the server before displaying anything in the console or recording anything in history, and that message type doesn't relay any information about whether or not the message should be stored in history

export interface JupyterExecuteInput extends JupyterMessageSpec {
	/** The code to be executed */
	code: string;

	/** The count of executions */
	execution_count: number;  // eslint-disable-line
}

It almost feels like Positron should be recording some kind of information about store_history when it sends out an ExecuteRequest, so it can react appropriately when getting the rebroadcast ExecuteInput from the server.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: debugger Issues related to Debugging duplicate This issue or pull request already exists enhancement New feature or request lang: r support
Projects
None yet
Development

No branches or pull requests

4 participants