wrap to terminal width #249
Conversation
We are using the prettyprinter library, which nicely wraps and indents the output to fit the screen, but we were using the default layout options, which assume that the screen is always exactly 80 characters wide. With a slightly sinful unsafePerformIO, we can use the terminal's actual width instead.
WalkthroughThe changes introduce a new function, Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -58,8 +62,21 @@ angles doc = text "⟨" <> align (group doc) <> "⟩" | |||
| vec :: Doc ann -> Doc ann | |||
| vec doc = text "[" <> align (group doc) <> "]" | |||
|
|
|||
| customLayoutOptions :: LayoutOptions | |||
| customLayoutOptions = unsafePerformIO $ do | |||
There was a problem hiding this comment.
Using unsafePerformIO is the most straightforward way to do it, but of course I expect that somebody will object and that I will have to thread a LayoutOptions variable everywhere.
There was a problem hiding this comment.
One concern is that the tests use pretty, so they might start failing depending on the terminal width. But I tried making my terminal very small and the tests still passed 🤷
There was a problem hiding this comment.
Oh, that's because it doesn't work. echo $COLUMNS prints the right value in the terminal, but lookupEnv "COLUMNS" returns Nothing!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/Pretty.hs (5 hunks)
Additional comments not posted (1)
src/Pretty.hs (1)
79-79: Conditional approval of changes toprettyandprettyEnv.The modifications to use
customLayoutOptionsalign with the PR's objectives to dynamically adjust the layout based on the terminal width. However, the approval is conditional on addressing the potential issues withunsafePerformIOas noted in the review ofcustomLayoutOptions.The changes are conditionally approved pending the resolution of the
unsafePerformIOusage.Also applies to: 89-89
| import System.Environment (lookupEnv) | ||
| import System.FilePath (takeFileName) | ||
| import System.IO.Unsafe (unsafePerformIO) | ||
| import Text.Read (readMaybe) |
There was a problem hiding this comment.
Review the new imports for environment handling and unsafe IO operations.
The addition of System.Environment and System.IO.Unsafe is necessary for the new functionality to dynamically fetch the terminal width. However, the use of unsafePerformIO should be carefully justified as it can lead to non-deterministic behavior in a purely functional setting.
Consider alternatives to unsafePerformIO if possible, to maintain the purity of the functions.
| customLayoutOptions :: LayoutOptions | ||
| customLayoutOptions = unsafePerformIO $ do | ||
| columnsMaybeString <- lookupEnv "COLUMNS" | ||
| let columnsMaybeInt :: Maybe Int | ||
| columnsMaybeInt = do | ||
| str <- columnsMaybeString | ||
| readMaybe str | ||
| let columns :: Int | ||
| columns = fromMaybe 80 columnsMaybeInt | ||
|
|
||
| pure $ defaultLayoutOptions | ||
| {layoutPageWidth = AvailablePerLine columns 1.0} | ||
|
|
There was a problem hiding this comment.
Consider refactoring customLayoutOptions to avoid unsafePerformIO.
The function uses unsafePerformIO to fetch the terminal width, which can lead to unpredictable behavior in Haskell. It's generally recommended to keep IO operations explicit in the function's type signature to maintain purity and predictability.
Consider refactoring the function to return IO LayoutOptions instead, and handle the IO operation in the calling context. This change would make the IO operations explicit and maintain the purity of the function.
customLayoutOptions :: IO LayoutOptions
customLayoutOptions = do
columnsMaybeString <- lookupEnv "COLUMNS"
let columnsMaybeInt = readMaybe =<< columnsMaybeString
let columns = fromMaybe 80 columnsMaybeInt
pure $ defaultLayoutOptions { layoutPageWidth = AvailablePerLine columns 1.0 }
|
Closing because it doesn't work 🙃 |
We are using the prettyprinter library, which nicely wraps and indents the output to fit the screen, but we were using the default layout options, which assume that the screen is always exactly 80 characters wide. With a slightly sinful unsafePerformIO, we can use the terminal's actual width instead.
Summary by CodeRabbit