-
-
Notifications
You must be signed in to change notification settings - Fork 410
Remove extra call to newHscEnvEqWithImportPaths #3676
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
Conversation
|
We also update the |
fendor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
Worth a comment explaining what's going on? |
|
|
|
put it in the code! |
| session' <- liftIO $ mergeEnvs hsc moduleNode inLoadOrder depSessions | ||
|
|
||
| Just <$> liftIO (newHscEnvEqWithImportPaths (envImportPaths env) session' []) | ||
| -- Here we avoid a call to to `newHscEnvEqWithImportPaths`, which creates a new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelpj Does this sufficiently explain what's going on here? Is there anything we should add?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say ask yourself:
- If I was reading this for the first time, would I know what is going on and why?
- Has this captured the knowledge that I gained in order to make the fix?
ff6d39e to
222b050
Compare
| session' <- liftIO $ mergeEnvs hsc moduleNode inLoadOrder depSessions | ||
|
|
||
| Just <$> liftIO (newHscEnvEqWithImportPaths (envImportPaths env) session' []) | ||
| -- Here we avoid a call to to `newHscEnvEqWithImportPaths`, which creates a new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Submitting this at the request of @wz1000
Instead of calling
newHscEnvEqWithImportPathsagain inghcSessionDepsDefinition, we only generate a newUniquefor theHscEnvEqand update theenvUniquefield.