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

[#89] Add 'putStdoutColoured' and 'putStderrColoured' #92

Merged

Conversation

Dponya
Copy link
Contributor

@Dponya Dponya commented Oct 14, 2022

Resolves #89

Additional tasks

  • Documentation for changes provided/changed
  • Tests added
  • Updated CHANGELOG.md

@Dponya Dponya requested a review from chshersh as a code owner October 14, 2022 11:09
@chshersh chshersh added the hacktoberfest-accepted https://hacktoberfest.com/participation/ label Oct 14, 2022
@chshersh
Copy link
Owner

Hi @Dponya 👋🏻

I see that @lillycat332 already opened a PR to resolve the same issue but they were slightly faster 😅

I still accept your contribution so it should be accepted for Hacktoberfest nevertheless 👍🏻

@Dponya
Copy link
Contributor Author

Dponya commented Oct 14, 2022

Hi @Dponya 👋🏻

I see that @lillycat332 already opened a PR to resolve the same issue but they were slightly faster sweat_smile

* [#90 - Changes types in 'Iris.Colour.Formatting' from 'ByteString' to 'Text' #91](https://github.com/chshersh/iris/pull/91)

I still accept your contribution so it should be accepted for Hacktoberfest nevertheless 👍🏻
@chshersh hey 👋

Oh, oops. Sorry for that, I thought there is two independent issues #90 and #89. One for replacing ByteString to Text, another for output issue in #87. If @lillycat332's PR solves problem with #87, should I close this PR?

@chshersh
Copy link
Owner

@Dponya My bad 🙏🏻 Indeed, I confused two issues 😮‍💨

In that case, I see no problem and no conflict! 👌🏻

Copy link
Owner

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

Thanks, looks great!

I have only minor suggestions. Also, could you please add your changes to the CHANGELOG in the [unreleased] section? 🙏🏻

"my message"
@

@since 0.0.0.0
Copy link
Owner

Choose a reason for hiding this comment

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

Let's specify the x.x.x.x version for all new functions in types. I'll run a command to update all versions before the next release when the actual release version is known

Suggested change
@since 0.0.0.0
@since x.x.x.x

@Dponya
Copy link
Contributor Author

Dponya commented Oct 15, 2022

Thanks, looks great!

I have only minor suggestions. Also, could you please add your changes to the CHANGELOG in the [unreleased] section? 🙏🏻

Should I add #89 only or #16 too to the CHANGELOG?

@chshersh
Copy link
Owner

@Dponya You can add #16 to the changelog as well 👍🏻
You can also use these new functions in the simple-grep tutorial (and update the tutorial output as well)

@Dponya Dponya requested a review from chshersh October 15, 2022 14:18
$ BSL.toStrict $ TLE.encodeUtf8 x
printLine x = Iris.putStdoutColoured
(Colourista.formatWith [TS.empty])
$ T.toStrict $ x `mappend` "\n"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There I used putStdoutColoured without Ln for stable purposes. When the API of putStdoutColouredLn will be changed from ByteString to Text, there could be a need to change simple-grep too. So, I just used \n

@chshersh chshersh added the documentation Improvements or additions to documentation label Oct 19, 2022
@chshersh chshersh changed the title resolves #89 add putStdoutColoured & putStderrColoured [#89] Add 'putStdoutColoured' and 'putStderrColoured' Oct 19, 2022
Copy link
Owner

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

Looks great 👏🏻

@chshersh chshersh merged commit 1b8234a into chshersh:main Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation hacktoberfest-accepted https://hacktoberfest.com/participation/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add 'putStdoutColoured' and 'putStderrColoured'
2 participants