search

ndmitchell / rattle

Forward build system with speculation and caching
Other
102 stars 5 forks source link

profiling for rattle #2

Closed spall closed 5 years ago

spall commented 5 years ago

Hi Neil,
This isn't perfect but I wanted to make sure I submitted a pull request by today so I could get your opinion. I copied some hidden modules from Shake so I could get the html report working quickly, but I'm not sure this was the best choice. Additionally I think you generated "shake.js" from typescript, but modifying the existing javascript let me get something presentable more quickly.

I copied from Shake: src/General/Cleanup, src/General/Paths, src/General/Template, and src/Paths

spall commented 5 years ago

Hi Neil, what else do you think needs to change to merge this?

ndmitchell commented 5 years ago

If you look at the CI there are a bunch of HLint suggestions: https://travis-ci.org/ndmitchell/rattle/jobs/524229146. With those fixed, I'll merge it. Thanks a lot for the changes.

ndmitchell commented 5 years ago

There are now warnings in Profile.hs - see https://travis-ci.org/ndmitchell/rattle/jobs/524499133#L842.

I'm not averse to adding {-# OPTIONS_GHC -w #-} to just that module, if you want to ignore them for now while you are developing it.

spall commented 5 years ago

Yeah, I think some of those unused things will probably be used in the future.

ndmitchell commented 5 years ago

And on to the next error (sorry it's a bit of a pain). It says:

writeProfile :: RattleOptions -> FilePath -> IO ()
graphData :: RattleOptions -> IO (Seconds, Seconds, Seconds)

You added those two functions as public exports to the Rattle API but didn't written any Haddock docs (-- | documentation) to go with them. Should they be public? My guess is writeProfile definitely should, but perhaps not graphData? If you do want them public add a comment as to what they do.

ndmitchell commented 5 years ago

Actually, it's pretty close, so I'll fix up those remaining checks.

ndmitchell commented 5 years ago

I've now pushed some additional patches that should make it pass the CI. Thanks for the contribution!