Skip to content
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

Use optparse-applicative instead of cmdargs #111

Merged
merged 9 commits into from Mar 11, 2017

Conversation

@kanghyojun
Copy link
Member

@kanghyojun kanghyojun commented Mar 11, 2017

Use optparse-applicative which one is suggested in #88. this PR close #88 as well.

Copy link
Member

@dahlia dahlia left a comment

README.md에 CLI 예제가 있는데 그쪽도 업데이트해야 합니다.

Loading

nirum.cabal Outdated
@@ -133,6 +134,7 @@ test-suite spec
, megaparsec
, mtl
, nirum
, optparse-applicative >=0.13.1 && <0.14
Copy link
Member

@dahlia dahlia Mar 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test-suite spec 쪽은 library 쪽 의존성 버전을 따라가면 되니까, 버전 명시 안해도 될 것 같습니다.

Loading

src/Nirum/Cli.hs Outdated
optsParser =
OPT.info
(OPT.helper <*> versionOption <*> programOptions)
(OPT.fullDesc <> OPT.progDesc "Nirum compiler." <>
Copy link
Member

@dahlia dahlia Mar 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

원래는 버전도 같이 표시했었는데 빠졌네요. 마침표는 없어도 될 것 같습니다.

Loading

src/Nirum/Cli.hs Outdated
programOptions :: OPT.Parser Opts
programOptions =
Opts <$> OPT.strOption
(OPT.long "outdir" <> OPT.short 'o' <> OPT.metavar "OUTDIR" <>
Copy link
Member

@dahlia dahlia Mar 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이거 metavar은 DIR이면 될 것 같습니다. Long option name은 output-dir 정도로 푸는 게 나아 보입니다.

Loading

src/Nirum/Cli.hs Outdated
OPT.help "out directory") <*>
OPT.strOption
(OPT.long "target" <> OPT.short 't' <> OPT.metavar "TARGET" <>
OPT.help "Target name") <*>
Copy link
Member

@dahlia dahlia Mar 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

설명은 “target language name” 정도로 적어주세요.

Loading

src/Nirum/Cli.hs Outdated
(OPT.long "target" <> OPT.short 't' <> OPT.metavar "TARGET" <>
OPT.help "Target name") <*>
OPT.strArgument
(OPT.metavar "PACKAGE" <> OPT.help "Package directory")
Copy link
Member

@dahlia dahlia Mar 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

마찬가지로 metavar은 DIR 정도가 적당할 것 같습니다.

Loading

src/Nirum/Cli.hs Outdated
, (&=)
)
import System.Console.CmdArgs.Default (def)
import Data.Monoid
Copy link
Member

@dahlia dahlia Mar 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 모듈에서 어떤 거 가져다 쓰는 거예요? import qualified로 하거나 필요한 것만 명시적으로 가져오는 게 좋을 듯하네요.

Loading

README.md Outdated
-V --version Print version information
--numeric-version Print just the version number
Usage: nirum [-v|--version] (-o|--output-dir DIR) (-t|--target TARGET) DIR
Nirum compiler0.3.0
Copy link
Member

@dahlia dahlia Mar 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

버전 앞에 띄어쓰기 넣어주세요

Loading

README.md Outdated
@@ -55,17 +55,17 @@ In order to compile a Nirum package (`examples/`) to a Python package:
For more infomration, use `--help` option:

$ nirum --help
Nirum Compiler 0.3.0
nirum - The IDL compiler and RPC/distributed object framework
Copy link
Member

@dahlia dahlia Mar 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“Nirum”은 고유명사니까 대문자로 시작을…

Loading

dahlia
dahlia approved these changes Mar 11, 2017
yjroot
yjroot approved these changes Mar 11, 2017
@kanghyojun kanghyojun merged commit 18533c9 into nirum-lang:master Mar 11, 2017
2 checks passed
Loading
@dahlia dahlia added this to Delivered in Sprint at Jeju in March 2017 Mar 13, 2017
dahlia added a commit that referenced this issue Mar 13, 2017
Fix regressive things PR #111 missed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants