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

Support block on first error mode #845

Merged
merged 19 commits into from
Aug 5, 2024
Merged

Support block on first error mode #845

merged 19 commits into from
Aug 5, 2024

Conversation

rtetley
Copy link
Collaborator

@rtetley rtetley commented Jul 24, 2024

Closes #537

val build_tasks_for : Document.document -> Scheduler.schedule -> state -> sentence_id -> Vernacstate.t * prepared_task list * state
val execute : state -> Vernacstate.t * events * bool -> prepared_task -> (state * Vernacstate.t * events * bool)
val build_tasks_for : Document.document -> Scheduler.schedule -> state -> sentence_id -> bool -> Vernacstate.t * prepared_task list * state * sentence_id option
val execute : state -> Vernacstate.t * events * bool -> prepared_task -> (state * Vernacstate.t * events * bool * bool)
Copy link
Member

Choose a reason for hiding this comment

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

maybe it is time to name these booleans ;-)

@gares
Copy link
Member

gares commented Jul 26, 2024

If you give a name to the type range.t option, eg blocking_error, then the api update are are self explanatory

@rtetley rtetley force-pushed the block-on-first-error branch from e566624 to d6682d1 Compare July 26, 2024 07:57
@@ -101,7 +103,7 @@ val get_proof : state -> Settings.Goals.Diff.Mode.t -> Position.t option -> Proo

val get_completions : state -> Position.t -> (completion_item list, string) Result.t

val handle_event : event -> state -> (state option * events)
val handle_event : event -> state -> bool -> (state option * events * Range.t option)
Copy link
Member

Choose a reason for hiding this comment

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

TODO: doc the bool and return type

@rtetley rtetley marked this pull request as ready for review July 29, 2024 13:10
"vscoq.proof.block": {
"scope": "window",
"type": "boolean",
"default": true,
Copy link
Member

Choose a reason for hiding this comment

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

This is a behavior change, better be advertised in the changelog/news

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I change it to false by default maybe ?

Copy link
Member

@gares gares Jul 29, 2024

Choose a reason for hiding this comment

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

I really don't know. Probably the new default is less surprising.

One thing I'd like to change is that, even if it is off, going down 1 sentence blocks. But I got used to that from the CoqIDE days, so I don't know it it is just me. I mean: if I say go there, and there is far away, I really want to go there. If I want to execute the next sentence, and if it fails, I want to stay where I am. So to me ALT-down should have the ~should_block_on_error:true always, while ALT-right should read the value from the settings.

Copy link
Member

Choose a reason for hiding this comment

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

I thought a bit more about my use case, and maybe also the one of Laurent. I think moving the caret in case of a failing step forward is bad, it really seems step fwd is special.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay ! Just changed it

Copy link
Member

@gares gares Jul 30, 2024

Choose a reason for hiding this comment

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

Now I don't don't have the time to test it, but IIRC on block on error you both stop and move the caret back.
The last part should not happen when step forward fails.

Here I use | for the caret when I do a step fwd (ALT-down).

1) rewrite foo.| (* fails, damn, I move back to fix the rule *)
2) rewrite {2}|foo. (* fails again *)
3) rewrite {2 3|}foo. (* works *)

I'd like the caret to stay where it is when 2 fails, but move after the . when 3 works.

I think step fwd is special since you use it when you "refine" a command, and you may do it with the caret in the middle of it. So moving the caret elsewhere, like at the previous ., is just going to piss the user off.

Copy link
Collaborator Author

@rtetley rtetley Jul 30, 2024

Choose a reason for hiding this comment

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

So if I understand correctly, the caret should not move at all on step forward when there is an error?

Copy link
Member

Choose a reason for hiding this comment

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

IMVHO yes, this is the desired behavior. I think proof general and CoqIDE do the same, but my muscle memory may fail me. CC @ybertot @thery

@rtetley rtetley force-pushed the block-on-first-error branch from 3404078 to 4b04c29 Compare August 5, 2024 08:05
@rtetley rtetley force-pushed the block-on-first-error branch from d8a88fa to a417bba Compare August 5, 2024 11:39
@rtetley rtetley merged commit b759376 into main Aug 5, 2024
24 checks passed
@rtetley rtetley deleted the block-on-first-error branch August 8, 2024 13:54
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.

Block on first error mode
2 participants