Skip to content

Conversation

@nojaf
Copy link
Member

@nojaf nojaf commented Feb 4, 2025

No description provided.

let newText =
c.name ^ " {\n"
^ (cases
^ (cases |> List.sort String.compare
Copy link
Member 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
Member

@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
Member 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.

@nojaf
Copy link
Member Author

nojaf commented Apr 26, 2025

Might no longer be worth it.
Use Format.err_formatter kids.

@nojaf nojaf closed this Apr 26, 2025
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