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

[client] debug setting #217

Closed
wants to merge 1 commit into from
Closed

Conversation

Alizter
Copy link
Collaborator

@Alizter Alizter commented Jan 22, 2023

Signed-off-by: Ali Caglayan alizter@gmail.com

CHANGES.md Outdated Show resolved Hide resolved
Copy link
Owner

@ejgallego ejgallego left a comment

Choose a reason for hiding this comment

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

I am not sure why we should bother the user with these flags, which are really internal.

Is there a way to hide these options by default?

Already the clutter in the settings panel is not good, so I'm unsure about this, seems like something it will impact all users a lot while the developers will barely benefit.

@ejgallego
Copy link
Owner

I guess we can expose a single debug flags, and this flag would for now only turn backtraces on, I don't see how the rest could be useful. WDYT?

@Alizter
Copy link
Collaborator Author

Alizter commented Jan 22, 2023

We don't have to have these settings available in the UI at all. We could just allow users to type the settings directly in their config. There is precedent for this kind of behavior in other extensions.

@ejgallego
Copy link
Owner

We don't have to have these settings available in the UI at all. We could just allow users to type the settings directly in their config. There is precedent for this kind of behavior in other extensions.

If that can be done then it's good. If not we should IMO only expose a generic debug.

@Alizter
Copy link
Collaborator Author

Alizter commented Jan 23, 2023

@ejgallego This now works, you can configure the debug settings in your settings.json. The UI only shows a small debug entry which tells you to edit from settings.json.

I had to change the order in which coq was initialized so it came after recieving the init settings from the config.

@Alizter Alizter force-pushed the ps/rr/debug_settings branch 2 times, most recently from 215e7f2 to c0ebe51 Compare January 23, 2023 23:09
@Alizter Alizter added this to the 0.1.4 milestone Jan 23, 2023
@Alizter Alizter marked this pull request as ready for review January 23, 2023 23:10
(* LSP Server server initialization *)
let workspace = lsp_init_loop ic oc ~cmdline in

let fb_queue = Coq.Protect.fb_queue in
let root_state = coq_init ~fb_queue ~bt in
Copy link
Owner

Choose a reason for hiding this comment

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

The order here has to go back to coq_init first, then workspace init after.

That's not a big deal workspace initialization can take care of the debug stuff.

But indeed backtraces are special, for example you often want them to be enabled before the lsp_init loop takes place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I don't do this then backtraces are not picked up from the settings. What is your suggestion for this?

Copy link
Owner

Choose a reason for hiding this comment

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

Have Workspace.apply set it too, you'll need to add the option to the particular workspace setting.

@Alizter
Copy link
Collaborator Author

Alizter commented Jan 26, 2023

I've gone for a more direct way of setting backtraces. I put it in the loop so if the setting is changed for example after loading the config, it is able to trace.

Didn't really understand your Workspace.apply suggestion.

I've also removed the useless -bt flag.

<!-- ps-id: c6aaf75b-0994-4ab7-a173-e9f77c462266 -->

Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
Copy link
Collaborator Author

Alizter commented Jan 27, 2023

Maybe I am not so happy with this in the end.

@Alizter Alizter closed this Jan 27, 2023
@ejgallego
Copy link
Owner

Didn't really understand your Workspace.apply suggestion.

Sorry I didn't explain it properly, see #247

@ejgallego
Copy link
Owner

I'd suggest we resurrect this PR, but only exposing a backtraces / debug flag in the vscode side.

@Alizter Alizter reopened this Jan 27, 2023
@Alizter
Copy link
Collaborator Author

Alizter commented Jan 27, 2023

@ejgallego Feel free to take over.

@ejgallego ejgallego changed the title debug settings [client] debug setting Jan 27, 2023
@ejgallego ejgallego closed this Jan 27, 2023
ejgallego added a commit that referenced this pull request Jan 28, 2023
This is a simpler version of #217

Co-authored-by: Ali Caglayan <alizter@gmail.com>
ejgallego added a commit that referenced this pull request Jan 28, 2023
This is a simpler version of #217

Co-authored-by: Ali Caglayan <alizter@gmail.com>
ejgallego added a commit that referenced this pull request Jan 28, 2023
This is a simpler version of #217

Co-authored-by: Ali Caglayan <alizter@gmail.com>
ejgallego added a commit that referenced this pull request Jan 28, 2023
This is a simpler version of #217

Co-authored-by: Ali Caglayan <alizter@gmail.com>
ejgallego added a commit to ejgallego/opam-repository that referenced this pull request Jan 29, 2023
CHANGES:

---------------------

 - Support for OCaml 4.11 (@ejgallego, ejgallego/coq-lsp#184)
 - The keybinding alt+enter in VSCode is now correctly scoped to be
   only active on Coq files (@artagnon, ejgallego/coq-lsp#188)
 - Support Unicode files (@ejgallego, ejgallego/coq-lsp#200, fixes ejgallego/coq-lsp#193, fixes ejgallego/coq-lsp#197)
 - The info view is now script enabled and does client-side
   rendering. It is also now bundled with esbuild as part of the build
   process (@artagnon, @ejgallego, ejgallego/coq-lsp#171)
 - The no-op `--std` argument to the `coq-lsp` binary has been
   removed, beware of your setup in the extension settings
   (@ejgallego, ejgallego/coq-lsp#208)
 - Settings for the VSCode extension are now categorized (@Alizter, ejgallego/coq-lsp#212)
 - `GoalAnswer`s now include the proof "stack" and better hypothesis
   information, changes are compatible with 0.1.3 `GoalAnswer` version
   (@ejgallego, ejgallego/coq-lsp#237)
 - Focus is now preserved when the info view pops up (@artagnon, ejgallego/coq-lsp#242,
   fixes ejgallego/coq-lsp#224)
 - In `_CoqProject`, `-impredicative-set` is now parsed correctly
   (@artagnon, ejgallego/coq-lsp#241)
 - InfoView is not written in React (@ejgallego, ejgallego/coq-lsp#223)
 - `debug` option in the client / protocol that will enable Coq's backtraces
   (@Alizter, @ejgallego, ejgallego/coq-lsp#217, ejgallego/coq-lsp#248)
 - Full document stats are now correctly computed on checking
   resumption, still cached sentences will display the cached timing
   tho (@ejgallego, ejgallego/coq-lsp#257)
 - Set Coq library name correctly (@ejgallego, ejgallego/coq-lsp#260)
 - `_CoqProject` file is now detected using LSP client `rootPath`
   (@ejgallego, ejgallego/coq-lsp#261)
 - Press `\` to trigger Unicode completion by the server. This
   behavior is configurable, with "off", "regular", and extended
   settings (@artagnon, @Alizter, ejgallego, ejgallego/coq-lsp#219).
ejgallego added a commit to ejgallego/opam-repository that referenced this pull request Jan 29, 2023
CHANGES:

---------------------

 - Support for OCaml 4.11 (@ejgallego, ejgallego/coq-lsp#184)
 - The keybinding alt+enter in VSCode is now correctly scoped to be
   only active on Coq files (@artagnon, ejgallego/coq-lsp#188)
 - Support Unicode files (@ejgallego, ejgallego/coq-lsp#200, fixes ejgallego/coq-lsp#193, fixes ejgallego/coq-lsp#197)
 - The info view is now script enabled and does client-side
   rendering. It is also now bundled with esbuild as part of the build
   process (@artagnon, @ejgallego, ejgallego/coq-lsp#171)
 - The no-op `--std` argument to the `coq-lsp` binary has been
   removed, beware of your setup in the extension settings
   (@ejgallego, ejgallego/coq-lsp#208)
 - Settings for the VSCode extension are now categorized (@Alizter, ejgallego/coq-lsp#212)
 - `GoalAnswer`s now include the proof "stack" and better hypothesis
   information, changes are compatible with 0.1.3 `GoalAnswer` version
   (@ejgallego, ejgallego/coq-lsp#237)
 - Focus is now preserved when the info view pops up (@artagnon, ejgallego/coq-lsp#242,
   fixes ejgallego/coq-lsp#224)
 - In `_CoqProject`, `-impredicative-set` is now parsed correctly
   (@artagnon, ejgallego/coq-lsp#241)
 - InfoView is not written in React (@ejgallego, ejgallego/coq-lsp#223)
 - `debug` option in the client / protocol that will enable Coq's backtraces
   (@Alizter, @ejgallego, ejgallego/coq-lsp#217, ejgallego/coq-lsp#248)
 - Full document stats are now correctly computed on checking
   resumption, still cached sentences will display the cached timing
   tho (@ejgallego, ejgallego/coq-lsp#257)
 - Set Coq library name correctly (@ejgallego, ejgallego/coq-lsp#260)
 - `_CoqProject` file is now detected using LSP client `rootPath`
   (@ejgallego, ejgallego/coq-lsp#261)
 - You can press `\` to trigger Unicode completion by the server. This
   behavior is configurable, with "off", "regular", and "extended"
   settings (@artagnon, @Alizter, ejgallego, ejgallego/coq-lsp#219).
ejgallego added a commit to ejgallego/opam-repository that referenced this pull request Jan 29, 2023
CHANGES:

---------------------

 - Support for OCaml 4.11 (@ejgallego, ejgallego/coq-lsp#184)
 - The keybinding alt+enter in VSCode is now correctly scoped to be
   only active on Coq files (@artagnon, ejgallego/coq-lsp#188)
 - Support Unicode files (@ejgallego, ejgallego/coq-lsp#200, fixes ejgallego/coq-lsp#193, fixes ejgallego/coq-lsp#197)
 - The info view is now script enabled and does client-side
   rendering. It is also now bundled with esbuild as part of the build
   process (@artagnon, @ejgallego, ejgallego/coq-lsp#171)
 - The no-op `--std` argument to the `coq-lsp` binary has been
   removed, beware of your setup in the extension settings
   (@ejgallego, ejgallego/coq-lsp#208)
 - Settings for the VSCode extension are now categorized (@Alizter, ejgallego/coq-lsp#212)
 - `GoalAnswer`s now include the proof "stack" and better hypothesis
   information, changes are compatible with 0.1.3 `GoalAnswer` version
   (@ejgallego, ejgallego/coq-lsp#237)
 - Focus is now preserved when the info view pops up (@artagnon, ejgallego/coq-lsp#242,
   fixes ejgallego/coq-lsp#224)
 - In `_CoqProject`, `-impredicative-set` is now parsed correctly
   (@artagnon, ejgallego/coq-lsp#241)
 - InfoView is not written in React (@ejgallego, ejgallego/coq-lsp#223)
 - `debug` option in the client / protocol that will enable Coq's backtraces
   (@Alizter, @ejgallego, ejgallego/coq-lsp#217, ejgallego/coq-lsp#248)
 - Full document stats are now correctly computed on checking
   resumption, still cached sentences will display the cached timing
   tho (@ejgallego, ejgallego/coq-lsp#257)
 - Set Coq library name correctly (@ejgallego, ejgallego/coq-lsp#260)
 - `_CoqProject` file is now detected using LSP client `rootPath`
   (@ejgallego, ejgallego/coq-lsp#261)
 - You can press `\` to trigger Unicode completion by the server. This
   behavior is configurable, with "off", "regular", and "extended"
   settings (@artagnon, @Alizter, ejgallego, ejgallego/coq-lsp#219).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants