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

-w/--watch option #104

Merged
merged 2 commits into from Jun 1, 2017

Conversation

@yjroot
Contributor

yjroot commented Mar 7, 2017

Fixes #91.

@yjroot yjroot added the typ:enhance label Mar 7, 2017

@dahlia dahlia moved this from Chosen to In progress in Sprint at Jeju in March 2017 Mar 7, 2017

@yjroot yjroot force-pushed the yjroot:watch-option branch 3 times, most recently from 26aa4be to cbe7851 Mar 11, 2017

@yjroot yjroot changed the title from WIP: -w/--watch option to -w/--watch option Mar 11, 2017

@yjroot yjroot requested review from Kroisse and heejongahn Mar 11, 2017

@dahlia dahlia moved this from In progress to In review in Sprint at Jeju in March 2017 Mar 13, 2017

, watching :: Bool
, building :: TFlag
, changed :: TFlag
}

This comment has been minimized.

@dahlia

dahlia Mar 13, 2017

Member

들여쓰기

This comment has been minimized.

@dahlia

dahlia Mar 13, 2017

Member

@yjroot @admire93 위쪽에 있는 Options와도 많이 겹치는 것 같은데, 둘이 어떻게 다른 거죠? 공통된 것은 합칠 방법 없을까요?

}

debounceDelay :: Int
debounceDelay = 1 * 1000 * 1000

This comment has been minimized.

@dahlia

dahlia Mar 13, 2017

Member

단위가 어떻게 되나요? 단위가 드러났으면 좋겠습니다. 상수명 뒤에 단위를 붙이거나, 아니면 type Millisecond = Int 뭐 이런 식으로 type alias한 다음에 해당 타입으로 지정하거나…

Left (CompileError errors) ->
forM_ (M.toList errors) $ \ (filePath, compileError) ->
die [qq|error: $filePath: $compileError|]
tryDie options [qq|error: $filePath: $compileError|]

This comment has been minimized.

@dahlia

dahlia Mar 13, 2017

Member

이 함수 안에서 반복해서 tryDie options ...를 호출하고 있는데, 차라리 이 함수의 wheretry' = tryDie options 만들어 두고 그걸 쓰게 하는 게 낫지 않을까요.

options@AppOptions { building = building'
, changed = changed'
}
event

This comment has been minimized.

@dahlia

dahlia Mar 13, 2017

Member

AppOptions의 닫는 중괄호 위치가 여는 중괄호보다 왼쪽에 있네요. 그리고 onFileChanged 함수의 인자들도 밑에 있는 가드와 같은 위치에 있어서 좀 핫갈리는 것 같습니다. 인자들은 차라리 onFileChanged 바로 뒤에 위치시키는 건 어떨까요? 이런 식으로:

onFileChanged :: AppOptions -> Event -> IO ()
onFileChanged options@AppOptions { building = building'
                                 , changed = changed'
                                 }
                                 event
(T.pack $ targetOption opts)
watch'
building'
changed'

This comment has been minimized.

@dahlia

dahlia Mar 13, 2017

Member

이 정도면 필드가 꽤 많으니 그냥 레코드 문법을 쓰는 게 좋을 것 같습니다.

@@ -184,5 +258,8 @@ main = do
OPT.strOption
(OPT.long "target" <> OPT.short 't' <> OPT.metavar "TARGET" <>
OPT.help "Target language name") <*>
OPT.switch
(OPT.long "watch" <> OPT.short 'w' <>
OPT.help "Watches files for changes and rebuilds") <*>

This comment has been minimized.

@dahlia

dahlia Mar 13, 2017

Member

3인칭 단수형으로 쓰지 말고 그냥 명령형으로 쓰면 될 것 같습니다. (다들 그렇게 쓰는 것 같네요.)

@yjroot yjroot force-pushed the yjroot:watch-option branch from cbe7851 to 830ae37 Mar 23, 2017

@dahlia

dahlia approved these changes Mar 23, 2017

@yjroot yjroot force-pushed the yjroot:watch-option branch 2 times, most recently from 7b7a50b to 7cb2879 Mar 31, 2017

@yjroot yjroot force-pushed the yjroot:watch-option branch from 7cb2879 to e65d42f Mar 31, 2017

import GHC.Exts (IsList (toList))

import qualified Data.ByteString as B
import qualified Data.Map.Strict as M
import qualified Data.Set as S
import qualified Data.Text as T
import Control.Concurrent (threadDelay)

This comment has been minimized.

@dahlia

dahlia Mar 31, 2017

Member

Control.Concurrent는 표준 라이브러리니까 위쪽으로 올려주세요.

import GHC.Exts (IsList (toList))

import qualified Data.ByteString as B
import qualified Data.Map.Strict as M
import qualified Data.Set as S
import qualified Data.Text as T
import Control.Concurrent (threadDelay)
import Control.Concurrent.STM (atomically, newTVar, readTVar, TVar, writeTVar)
import Control.Monad (forM_, forever, when)

This comment has been minimized.

@dahlia

dahlia Mar 31, 2017

Member

Control.Monad는 표준 라이브러리니까 위쪽으로 올려주세요.

import System.FilePath (takeDirectory, (</>))
import System.FilePath (takeDirectory, takeExtension, (</>))
import System.FSNotify
import System.IO

This comment has been minimized.

@dahlia

dahlia Mar 31, 2017

Member

System.IO는 표준 라이브러리니까 위쪽으로 올려주세요.

@dahlia

dahlia approved these changes Jun 1, 2017

@Kroisse

Kroisse approved these changes Jun 1, 2017

@kanghyojun kanghyojun merged commit 5a1caf0 into nirum-lang:master Jun 1, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dahlia dahlia added the cmp:frontend label Aug 26, 2017

@dahlia dahlia moved this from In review to Delivered in Sprint at Jeju in March 2017 Dec 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment