Skip to content

Move Recorder to hls-plugin-api #3714

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 18 commits into from
Jul 23, 2023

Conversation

joyfulmantis
Copy link
Collaborator

@joyfulmantis joyfulmantis commented Jul 12, 2023

Moves the Recorder to hls-plugin-api, and implements logging in the Ide.Plugin.Resolve module.

@michaelpj
Copy link
Collaborator

As I said on IRC, I think a bunch of the functions in hls-plugin-api where we want to log are already in LspM and so MonadIO. For combineResponses yeah, we might want to use something else.

@joyfulmantis joyfulmantis requested a review from michaelpj July 18, 2023 19:10
Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

I think we just need to think about what logging policy we want here. As I said in the comment, I think we shouldn't need to additionally log if we are throwing, since I would expect those all to be recorded generically. So then the question is when do we want to log? I think one answer is "if we make a decision that might not be obvious to the user", or if something interesting or unexpected happens and we're not throwing.

@joyfulmantis joyfulmantis enabled auto-merge (squash) July 22, 2023 17:30
@joyfulmantis joyfulmantis added the merge me Label to trigger pull request merge label Jul 22, 2023
@joyfulmantis joyfulmantis merged commit b6dc425 into haskell:master Jul 23, 2023
@fendor fendor mentioned this pull request Aug 8, 2023
19 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants