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

Refactor Debug log in analysis #7276

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nojaf
Copy link
Collaborator

@nojaf nojaf commented Feb 4, 2025

No description provided.

@@ -2255,7 +2234,7 @@ let rec processCompletable ~debug ~full ~scope ~env ~pos ~forHover completable =
user has already written. Just complete for the rest. *)
let newText =
c.name ^ " {\n"
^ (cases
^ (cases |> List.sort String.compare
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't really explain this one but at least sorting should make this deterministic.

@nojaf nojaf marked this pull request as ready for review February 4, 2025 14:19
Copy link
Collaborator

@zth zth left a comment

Choose a reason for hiding this comment

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

As mentioned in chat, the reason for this being an if statement in the first place is to make it easy to avoid doing unnecessary computation unless actually in debug mode.

While we can achieve both probably by re-adding Debug.verbose as Debug.isVerboseMode or similar, I'd still like to understand a little bit more about why this change should be made. So, what are we gaining here more specifically?

@nojaf
Copy link
Collaborator Author

nojaf commented Feb 6, 2025

Okay, my rationale for this PR:

I'll start with a brief rant—feel free to skip this part!
Making sense of the completion analysis can be very frustrating. For starters, you can't just debug the code and step through it like you would in other languages. This seems to be an OCaml issue, and I don't quite understand why this isn't addressed.

Secondly, understanding the code can be quite challenging. Many functions and types would benefit from additional documentation comments. I would also advocate for more type annotations to facilitate navigation through the codebase. These issues often lead me to add numerous print_endline or Format.printf statements to figure out what's going on.

Now, onto some specific points:

  • Adding additional log statements can be very insightful, but it's risky when they're not inside an if verbose() then block. Forgetting this can clutter the stdout and cause the LSP server to malfunction. It isn’t always obvious when this occurs. LSP logs won’t indicate the problem, vscode will try to connect five times, leaving me clueless about why things aren’t working anymore.
  • Format.printf is useful, but I often forget to add the trailing \n, so I found the helper wrapper to be quite clever.
  • I don’t fully understand the “unnecessary computation” argument; Format.ifprintf seems like a no-op based on the documentation.

In summary, I want a safe way to log messages when verbose. I prefer not to clutter the code with if then statements, as it’s easy to overlook them, leading to confusing situations.

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.

2 participants