-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
resolves #16 write usage example using Literate Haskell #16 #87
resolves #16 write usage example using Literate Haskell #16 #87
Conversation
@chshersh Hi! Are there some problems with PR or CI pipelines? I've checked building of Iris on several ghc versions that were included into build, everything goes fine locally. Should I fix something? |
Hi @Dponya 👋🏻 Sorry, I haven't reviewed this PR yet. I've been very busy with personal stuff and tons of other PRs during Hacktoberfest 🥴 From CI logs, I can see that the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tutorial looks great! There's always room for improvement but I left a few main suggestions on how to improve the docs 🙂
And see my comment before on fixing CI.
examples/simple-grep/README.md
Outdated
```haskell | ||
{-# LANGUAGE OverloadedStrings #-} | ||
{-# LANGUAGE NamedFieldPuns #-} | ||
|
||
|
||
module Main (main) where | ||
|
||
import Prelude hiding (readFile) | ||
import Control.Monad.IO.Class (MonadIO (..)) | ||
import Control.Monad.Reader (MonadReader) | ||
|
||
import qualified Data.ByteString.Lazy as BSL | ||
import qualified Data.ByteString as BS | ||
import qualified Data.Text.Lazy as T | ||
import qualified Data.Text.Lazy.Encoding as TLE | ||
import qualified Options.Applicative as Opt | ||
|
||
import qualified Colourista | ||
import qualified Iris | ||
import qualified Paths_iris as Autogen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, this part is called "Preamble" in LHS tutorials. So it's beneficial to move all language extensions and imports into a separate ## Preamble
section and separate code block.
I would go even further and probably moved Iris
import and Paths_iris
import into separate sections with extra comments.
examples/simple-grep/README.md
Outdated
There is a basic example usage of Iris and a tutorial-like explanation: | ||
|
||
First of all, let's create a newtype wrapper for the main monad of Iris: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's give a link to grep
and ripgrep
to give an idea of what we want to build. It would be also beneficial to provide a usage example to immediately say the expected behaviour:
$ simple-grep --path=README.md --str=contribut
<and the output goes here>
examples/simple-grep/README.md
Outdated
{ | ||
filePath :: !String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor formatting improvement
{ | |
filePath :: !String | |
{ filePath :: !String |
examples/simple-grep/README.md
Outdated
, Iris.cliEnvSettingsCmdParser = SimpleGrep | ||
<$> Opt.strOption | ||
( Opt.long "filePath" | ||
<> Opt.short 'f' | ||
<> Opt.metavar "PATH" | ||
<> Opt.help "filePath to find and grep file" | ||
) | ||
<*> Opt.strOption | ||
( Opt.long "substring" | ||
<> Opt.short 's' | ||
<> Opt.help "Substring to find and highlight" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move CLI parser into a separate section combined with the SimpleGrep
definition so the appSettings
definition will be even shorter
``` | ||
|
||
We describe settings for the app based on `defaultCliEnvSettings`. It helps when we don't need some option to describe, so we can just skip it. Here we can write a CLI-program description, put required tools with a list and then pass commands with the `Parser a` type from `optparse-applicative` to the `cliEnvSettingsCmdParser` field. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also add how the result of the --help
looks like so people can see the UX.
examples/simple-grep/README.md
Outdated
occurences :: [T.Text] -> T.Text -> [(Int, T.Text)] | ||
occurences lines substring = go lines 0 | ||
where | ||
go (x:xs) idx | substring `T.isInfixOf` x = (idx, x) : go xs (idx + 1) | ||
| otherwise = go xs (idx + 1) | ||
go [] _ = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a simpler way to implement this function. I would write something like this:
occurences :: [T.Text] -> T.Text -> [(Int, T.Text)] | |
occurences lines substring = go lines 0 | |
where | |
go (x:xs) idx | substring `T.isInfixOf` x = (idx, x) : go xs (idx + 1) | |
| otherwise = go xs (idx + 1) | |
go [] _ = [] | |
occurences :: T.Text -> [T.Text] -> [(Int, T.Text)] | |
occurences str = filter (\(_lnNum, txt) -> str `T.isInfixOf` txt) . zip [1 .. ] |
examples/simple-grep/README.md
Outdated
liftIO $ printIdx idx | ||
liftIO $ putStr ":" | ||
printLine line | ||
|
||
printIdx idx = putStr $ " " ++ show idx | ||
|
||
printLine :: T.Text -> App () | ||
printLine x = Iris.putStdoutColouredLn | ||
(Colourista.formatWith [Colourista.yellow, Colourista.bold]) | ||
$ BSL.toStrict $ TLE.encodeUtf8 x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose a slightly different change:
Let's write the number and :
to stderr
using putStderrColouredLn
and the rest with just putStdoutColouredLn
and no colouring. This way, if you redirect simple-grep
output to another tool, you'll get only text lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great, and I did it. But, it's okay that output there now like this structure? There is a new line after the number and :
. It cause of putStdoutColoured
or putStderrColoured
doesn't exists. Maybe if it's not okay, there exists a solution for that. I'm not so well familiar with the terminal's escaping codes 🙌.
cabal exec simple-grep -- -f /some/dir/iris/iris.cabal -s iris
Starting grepping 🔥
file name: iris.cabal
2:
name: iris
7:
See [README.md](https://github.com/chshersh/iris#iris) for more details.
8:
homepage: https://github.com/chshersh/iris
9:
bug-reports: https://github.com/chshersh/iris/issues
26:
location: https://github.com/chshersh/iris.git
79:
build-depends: , iris
119:
autogen-modules: Paths_iris
120:
other-modules: Paths_iris
123:
, iris
136:
autogen-modules: Paths_iris
137:
other-modules: Paths_iris
150:
test-suite iris-test
160:
Paths_iris
164:
, iris
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I see the problem. Those functions output line break at the end of line so they are not suitable for this case. I've created a separate issue to implement the required functions:
Meanwhile, feel free to keep the version you're the most happy with and we can always update the tutorial later 😌
Thanks! Truly talk, I was unsatisfied with the quality of the documentation. So, I took look at the So, I pushed new fixes that maybe fit the above proposals. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this version of the tutorial much better! Thank, that's a great improvement 👍🏻
…ple-usage-with-literate-haskell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks great! 👏🏻
Really nice to see such documentation improvements for Iris 🙂
Resolves #16
Well. It seems like that my current implementation have problems with requirement in issue:
lines numbers to stderr and with lines to stdout (to mimic the output of ripgrep)
I've seen a
ripgrep
and implemented it like that:And in this function I used
putStr
for output like that:Where the line number is putted right after
Iris.putStdoutColouredLn
output. Maybe it breaks requirements withstderr
andstdout
, if it is, how to avoid it? I really don't have an idea to implement it in another way to include using Colourista to print it the right way with colors.And I'm sure that the documentation in
examples/simple-grep/README.md
has serious problems with explanation, if it is, please leave in the comments proposals to fix it.Thanks you!