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

Customize the command printing function #228

Merged
merged 2 commits into from
Apr 3, 2023

Conversation

chrismwendt
Copy link
Contributor

@chrismwendt chrismwendt commented Mar 31, 2023

Useful for adding color or a prefix like "Running command: " <> cmd <> " ...".

Copy link
Collaborator

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Please add test and investigate backwards compatibility, so a decision can be made whether this PR should be merged.

src/Shelly.hs Outdated
@@ -1284,7 +1290,7 @@ runHandles exe args reusedHandles withHandles = do
state <- get

let cmdString = show_command exe args
when (sPrintCommands state) $ echo cmdString
when (sPrintCommands state) $ liftIO $ sPrintCommandsFn state cmdString
Copy link
Collaborator

Choose a reason for hiding this comment

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

Switching from echo to sPrintCommandsFn with standard semantics TIO.hPutStrLn does not seem to preserve the current behavior, see the current definition of echo:

echo msg = traceEcho msg >> liftIO (TIO.putStrLn msg >> hFlush stdout)

@chrismwendt
Copy link
Contributor Author

Added a test, and I'm pretty sure it's backwards compatible because previously it was TIO.putStrLn and now the default isTIO.hPutStrLn stdout, which is the definition of TIO.putStrLn:

-- | Write a string to 'stdout', followed by a newline.
putStrLn :: Text -> IO ()
putStrLn = hPutStrLn stdout

https://github.com/haskell/text/blob/1.2.4.1/src/Data/Text/IO.hs#L314-L316

Copy link
Collaborator

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Thanks for the test!
Also, now the semantics of echo is preserved.

AFAICS, State is an internal data structure not part of the API, so the API is only extended by a new function, not changed, so this could be released as a minor version (1.12.1). Is that right?

Could you formulate a sentence for the changelog?

@andreasabel andreasabel added this to the 1.12.1 milestone Apr 2, 2023
@andreasabel andreasabel self-assigned this Apr 2, 2023
@chrismwendt
Copy link
Contributor Author

Added a changelog entry. This adds an option to the API, so users don't have to change their code when they upgrade.

@andreasabel
Copy link
Collaborator

So after merging your other PR (#229), there are conflicts...

@chrismwendt
Copy link
Contributor Author

I'm on it

@chrismwendt chrismwendt force-pushed the custom-command-printing branch 2 times, most recently from daa2d1d to a7f36a5 Compare April 2, 2023 08:37
@chrismwendt
Copy link
Contributor Author

Fixed conflicts

Copy link
Collaborator

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

Sorry, I have to request further changes. I remembered that there is also the API Shelly.Lifted which should evolve in sync with Shelly. Could you please extend this API by the new functionality? (That would be echoWith and print_commands_with, I think.)

ChangeLog.md Outdated Show resolved Hide resolved
@chrismwendt
Copy link
Contributor Author

Updated Lifted

Useful for adding color or a prefix like `"Running command: " <> cmd <> " ..."`.

New in API: `echoWith`, `print_commands_with`
Copy link
Collaborator

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Thanks!

I squashed the commits and bumped the version.

Candidate at: https://hackage.haskell.org/package/shelly-1.12.1/candidate/changelog

@andreasabel andreasabel merged commit db62da9 into gregwebs:master Apr 3, 2023
@andreasabel
Copy link
Collaborator

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

Successfully merging this pull request may close these issues.

None yet

2 participants