search

fpco / ide-backend

ide-backend drives the GHC API to build, query, and run your code
120 stars 17 forks source link

*edit* doesn't affect external API regarding session updates. #286

Closed urbanslug closed 9 years ago

urbanslug commented 9 years ago

Not ready for merge

I removed all the uses of ideSessionSourceDir and ideSessionDataDir. I now have the functions fetchSourceDir and fetchDataDir. It is able to automatically discover source and data files in the current working directory as shown here: https://github.com/urbanslug/bae/blob/master/Backend.hs

However, it isn't able to work in a server environemt yet because in IdeSession.Update.IdeSessionUpdate it was a hack since I replaced ideSessionSourceDir and ideSessionDataDir with idPath which is just id.

To really replace them I would have to make changes that affect the external API as far as modules doing session updates are concerned. Meaning:

Which do you think would be the best way to go about this?

As the code is right now it works it doesn't work in a server centric environment.

Note: ide-backend-server still uses ideSessionSourceDir and ideSessionDataDir.

urbanslug commented 9 years ago

Hey @snoyberg @mgsloan could you look at this? Only the second last commit matters. I should've squashed them into one. The commit message just echo a lot of what I've said.

urbanslug commented 9 years ago

So I found a way to replace all uses of ideSessionSourceDir and ideSessionDataDir without affecting the external API. It's not yet ready for merge since it has not yet fulfilled all the conditions of issue https://github.com/fpco/ide-backend/issues/280 specifically the one regarding recompiles.

I don't see the need for throwing errors with the use of "updateSourceFile" / "updateDataFile" and such other functions since it's able to discover source and data files in the current working directory. This means the user need not even use such functions, is there anything I'm missing?

snoyberg commented 9 years ago

I discussed this separately with @urbanslug to get a better idea of how this would work. What I realized is that I'm only now coming up to speed on how big an impact this PR (and @mgsloan's ideas from #280) will have. Now I'm starting to really like it, it seems like this is obviously (for some values of obvious) the right approach for the local editing case.

I'd like to see some test cases added for the new config value, in particular showing that when the file is modified locally and an recompilation is triggered, it actually recompiles. Other than that, I defer to @mgsloan, who's already demonstrating good leadership on this point so I won't get in the way :).

urbanslug commented 9 years ago

@snoyberg @mgsloan What do you guys think so far? Only problem is the working directory gets polluted with rpc.* directories. Otherwise I think it's ready for merge.

mgsloan commented 9 years ago

Hi @urbanslug! I finally got around to taking a look at this, sorry for the delay. I've added some comments here: https://github.com/fpco/ide-backend/pull/286/files

To move this forward, please address those comments and squash this into two commits. In particular, one commit for the change to .gitignore and one for the rest of the changes. Ideally, the main change should be one commit because the different changes aren't useful on their own - they're interdependent.

Maybe you're already doing this, since sofar my test run has passed, however, also please make sure the test suite passes! Here's how to run the test-suite: 

cabal configure --enable-tests
cabal build
./dist/build/TestSuite/TestSuite --test-78

(Note: if you're using a different version of GHC, then use a different number after --test-)

urbanslug commented 9 years ago

I probably screwed a few version control things while squashing and rearranging the order of commits.