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

Formalize the ProgressReporting Type #4335

Merged
merged 11 commits into from
Jun 30, 2024

Conversation

soulomoon
Copy link
Collaborator

@soulomoon soulomoon commented Jun 25, 2024

follow up of #4205
This is intended to remove the useless inProgress when tracking the inProgressState from the outside.

  1. Add kind data ProgressStateTrackingType = ProgressStateInsideTracking | ProgressStateOutsideTracking to distinguish the two type of inProgressState tracking.
  2. Spread the TProgressState to related type location to mark what kind of tracking we used. Then we won't have inProgress for ProgressStateOutsideTracking

@soulomoon soulomoon requested a review from wz1000 as a code owner June 25, 2024 16:08
@soulomoon soulomoon requested a review from michaelpj June 25, 2024 22:57
@michaelpj
Copy link
Collaborator

I'm not sure I see the attraction of this design, to be honest. I think we could instead see the tracking with files as a wrapper on top of the simpler progressCounter that just works with the STM Ints. That would mean we have two different types for the two different counters, rather than just one with a type parameter, but I think it would be simpler?

@soulomoon soulomoon force-pushed the soulomoon/formlization-of-ProgressReporting branch from 3b2e5d9 to 20b4304 Compare June 28, 2024 17:15
@soulomoon
Copy link
Collaborator Author

soulomoon commented Jun 28, 2024

Good suggestions @michaelpj ,
I've managed to change it to the wrapper version,
Seems much simpler using the wrapper version here.

-- ^ see Note [ProgressReporting API and InProgressState]
progressStop :: IO ()
data ProgressReportingNoTrace m = ProgressReportingNoTrace
{ progressUpdateI :: ProgressEvent -> m (),
Copy link
Collaborator

Choose a reason for hiding this comment

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

You might not need to add name suffixes if you use DuplicateRecordFields?

Copy link
Collaborator Author

@soulomoon soulomoon Jun 28, 2024

Choose a reason for hiding this comment

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

Nop, we have class function and data field collision. DuplicateRecordFields is not helpful here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But maybe we can make it prefix with '_' and not export it.

ghcide/src/Development/IDE/Core/ProgressReporting.hs Outdated Show resolved Hide resolved
}

class ProgressReportingClass a where
type M a :: * -> *
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I asked this before, but can everything just be in IO? The only place we use another monad is in inProgress, and I think we could also just provide that with IO and get people to use MonadUnliftIO to use it?

Copy link
Collaborator Author

@soulomoon soulomoon Jun 28, 2024

Choose a reason for hiding this comment

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

Yes, we can turn them into IOs and save the m for type cleaness, but might introuce a bit runtime overhead?

ghcide/src/Development/IDE/Core/ProgressReporting.hs Outdated Show resolved Hide resolved
@michaelpj
Copy link
Collaborator

Looks much better

@soulomoon soulomoon requested a review from michaelpj June 29, 2024 09:27
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.

Great

@michaelpj michaelpj enabled auto-merge (squash) June 30, 2024 15:49
@michaelpj michaelpj merged commit 495af1f into master Jun 30, 2024
36 checks passed
dsaenztagarro added a commit to dsaenztagarro/haskell-language-server that referenced this pull request Jun 30, 2024
* master:
  Formalize the ProgressReporting Type (haskell#4335)
  fix future index time (haskell#4343)
  Cleanup disabled warnings (haskell#4341)
  Cleanup imports after CPP removal + few hlint fixes (haskell#4337)
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.

None yet

2 participants