Build psciEnvironment as needed for :print in REPL - #3241
Conversation
- Complete all names that have been imported (transitively or directly) - Do not complete names that haven't been imported - Only recompute list of names after import or adding a let binding rather than after each request for name completion This commit fixes #3227
|
Ok, this is ready for review. I've also used (Prior build errors were due to (a) using |
| | inputPath `isSuffixOf` x = findReplace xs acc (n+1) | ||
| | otherwise = findReplace xs acc n | ||
| findReplace [] _ 0 = Nothing | ||
| findReplace [] acc _ = Just (newPath : reverse acc) |
There was a problem hiding this comment.
This function seems kind of odd to me, but otherwise this PR seems like a win. Nothing is ever done with acc, so I'm actually not sure what all this indirection is about.
There was a problem hiding this comment.
It's been a while, but I believe this function takes a list of paths and removes all that have matching suffixes and sticks the newPath in front, only if there were matches. If no matches were found, it returns Nothing. On the last line (where at least one path matched), acc is the list of paths that remained, because they didn't match, in reverse order.
I think the bigger picture is to make a new temporary file to replace the module we want to "edit". The module's path is replaced with the path to the new temporary file that contains the replacement module definitions.
There was a problem hiding this comment.
But you never case or cons on acc. As far as I can tell, acc is only ever [].
There was a problem hiding this comment.
If that isn't a bug, and acc is indeed unused, then I think this can just be a fold. (It can probably just be a fold regardless).
There was a problem hiding this comment.
Oh, yes. Shows how it's easy to be blind to your own bugs.
There was a problem hiding this comment.
Using a fold will probably be simpler, but maybe it will have unintended consequences later on. Your thoughts?
There was a problem hiding this comment.
Would it be possible to import and edit a small example included in the test directory, rather than a library?
Using a fold will probably be simpler, but maybe it will have unintended consequences later on. Your thoughts?
I'm not sure exactly what you mean. I think the main thing is that it's just working as intended. I could maybe offer a suggestion given the fixed implementation.
There was a problem hiding this comment.
Let me know if the implementation of findReplace can be simplified, but I don't think the original definition was correct (it should have cons'd ontoacc).
I took your suggestion and made a standalone module in tests/support/psci/Reload.purs and the edited version is Reload.edit. I also brought this branch up-to-date with master.
There was a problem hiding this comment.
I think the only reason you have it return Maybe is for a sanity check, otherwise you can just do it with fmap. I think it's fine. I might use a Boolean instead of Int since you only check against 0.
There was a problem hiding this comment.
Yes, I couldn't talk myself into getting rid of the sanity check after it did catch a mistake when I came back to this PR after many months. But I think I've made it better in the last commit.
natefaubion
left a comment
There was a problem hiding this comment.
This appears to do what it says on the tin 😄
|
Thanks! |
This fixes #3001 by computing
psciEnvironmenton demand.(The above issue is handled, see comment below)
Also, it should be noted that any code that calls
Language.PureScript.Interactive.makedepends on and modifies global state (since it reads/writes files in.psci_modules); I believe that includes all the other PSCi tests. This was a painful lesson that took me a while to learn, maybe others will benefit from this comment.