feat: added a custom writer for all text printers #324

Brookke · 2022-03-17T16:49:59Z

Description

Add custom writer support for most of the printers except for the area_printer

Scope

What is affected by this pull request?

Bug Fix
New Feature
Documentation
Other

Related Issue

Fixes #261

To-Do Checklist

I tested my changes
I have commented every method that I created/changed
I updated the examples to fit with my changes
I have added tests for my newly created methods

MarvinJWendt · 2022-03-17T22:06:16Z

Hi @Brookke, just a quick info: The snapshot testing is currently behaving a little weird, so the Build action will most likely always fail. If your local test runs succeed, it's fine. I will fix the tests for this PR when you're ready :)

Brookke · 2022-03-17T23:05:48Z

Hi @Brookke, just a quick info: The snapshot testing is currently behaving a little weird, so the Build action will most likely always fail. If your local test runs succeed, it's fine. I will fix the tests for this PR when you're ready :)

Seeing a similar snapshot failure locally too, so I will try and get that sorted

codecov · 2022-03-17T23:22:36Z

Codecov Report

Merging #324 (48050e3) into master (091ef5d) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #324   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines         1473      1505   +32     
=========================================
+ Hits          1473      1505   +32

Impacted Files	Coverage Δ
barchart.go	`100.00% <100.00%> (ø)`
basic_text_printer.go	`100.00% <100.00%> (ø)`
bigtext_printer.go	`100.00% <100.00%> (ø)`
box_printer.go	`100.00% <100.00%> (ø)`
bulletlist_printer.go	`100.00% <100.00%> (ø)`
center_printer.go	`100.00% <100.00%> (ø)`
header_printer.go	`100.00% <100.00%> (ø)`
panel_printer.go	`100.00% <100.00%> (ø)`
paragraph_printer.go	`100.00% <100.00%> (ø)`
prefix_printer.go	`100.00% <100.00%> (ø)`
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 091ef5d...48050e3. Read the comment docs.

Brookke · 2022-03-17T23:29:16Z

@MarvinJWendt When you get a moment in the next few days mind giving this a partial review. What this PR does is builds upon Florians previous work, however rather than setting a default io.Writer for every printer it leaves them as nil, and simply falls back to the default one set in color if the writer is nil, this way the existing tests all still work as expected and it remains backwards compatible.

If you are happy with this approach I will then move on to start writing tests for each of the new WithCustomWriter functions.

The only thing I am not super confident about are the writer check lines here: https://github.com/pterm/pterm/pull/324/files#diff-a771b53b10418ad32cd676b8f07bed1b1bd9bbc7ee430bec4b7172c0de679e61R109 however I will try to do some local testing on these to see if this works as expected.

Brookke · 2022-03-17T23:32:04Z

The only thing I am not super confident about are the writer check lines here: https://github.com/pterm/pterm/pull/324/files#diff-a771b53b10418ad32cd676b8f07bed1b1bd9bbc7ee430bec4b7172c0de679e61R109 however I will try to do some local testing on these to see if this works as expected.

upon second thoughts I remember we said I'd deal with this in a second PR so I will remove the LivePrinters ability to use a custom writer

MarvinJWendt · 2022-03-17T23:51:09Z

I will review this shorty!
I am currently removing the snapshot tests, they are just way too buggy at the moment, and trigger more false positives than real failures. This will make testing easier for you too :)

MarvinJWendt · 2022-03-18T00:15:19Z

Snapshot testing is now removed.

Note: if you use any JetBrains IDE (IntelliJ IDEA, GoLand, etc.) you should commit and push your changes before pulling master into your branch, as we also removed the .idea folder from the repo, which will result in IntelliJ breaking. The fix is to re-clone the project and it should just work fine again. as IntelliJ will re-generate the folder locally then.

MarvinJWendt

Looks very good! I have noted a small change (rename WithCustomWriter to WithWriter), but that's all. Awesome work, thanks a lot!

Oh one small thing, when you're done, please also update the docs for https://pterm.sh by editing /docs/printers/*. Just append the new functions to the list, that should be enough!

basic_text_printer.go

MarvinJWendt · 2022-03-18T00:20:23Z

basic_text_printer.go

-// Print formats using the default formats for its operands and writes to standard output.
+// Print formats using the default formats for its operands and writes to provided writer.


Nice catch!

Brookke · 2022-03-18T09:15:17Z

I will review this shorty!
I am currently removing the snapshot tests, they are just way too buggy at the moment, and trigger more false positives than real failures. This will make testing easier for you too :)

For what it's worth I thought the snapshot testing had some value as it did catch some of the issues I had, and they did run locally and pass for me. The main issue I had with them is that they would keep creating snapshot files each time I ran then and I would have to clear those between tests.

MarvinJWendt · 2022-03-18T22:19:59Z

I will review this shorty!
I am currently removing the snapshot tests, they are just way too buggy at the moment, and trigger more false positives than real failures. This will make testing easier for you too :)

For what it's worth I thought the snapshot testing had some value as it did catch some of the issues I had, and they did run locally and pass for me. The main issue I had with them is that they would keep creating snapshot files each time I ran then and I would have to clear those between tests.

In a few cases PTerm renders different output in the CI systems than locally, which has caused just about every CI run to fail. I am planning a bigger refactor, we'll probably add it back then!

MarvinJWendt · 2022-03-22T20:05:25Z

Hi @Brookke, is the PR ready for review? If so, please mark it as "Ready for review" :)

Brookke · 2022-03-23T18:25:23Z

Sorry haven't added any tests yet, let me try and get those sorted tomorrow!

…sic text printer

Brookke · 2022-03-23T18:51:26Z

Okay should be good to go now

MarvinJWendt

Description	Passed
Every `WithXXX(...)` function returns a pointer of the parent struct.	YES
Test contains nil check.	YES
Printers implement the right interface.	YES
Variable and function names describe what they do.	YES
Printer contains theme support.	N/A
Styles in structs must be pointers.	N/A
Printer tests uses correct test template.	YES
Commit messages follow conventional commit style. `If NO -> Squash`	NO
Possible errors are declared in the `errors.go` file.	N/A
Theme styles do not return pointers	N/A
Documentation in `./docs` is updated correctly.	YES

MarvinJWendt · 2022-03-28T15:40:32Z

Looks very good! Huge thanks for the contribution! I am currently doing some tests with it locally. If everything works as expected (I highly doubt it won't), it will be released today! I also gave you the contributor role in our Discord 😉

MarvinJWendt · 2022-03-28T16:06:03Z

Hi @Brookke, sorry the release will be a little delayed. GitHub Pages won't build, and after talking to the GitHub Support, it seems that they're having issues.

As soon as the docs site is up, the release will be made!

Brookke · 2022-03-28T16:10:41Z

Thank you! No rush ☺️

MarvinJWendt · 2022-03-28T21:32:58Z

Released in v0.12.40! 🚀

github-actions bot added the feature label Mar 17, 2022

Brookke force-pushed the issue-261-Add_ability_to_set_custom_io_writter_for_each_of_the_different_output_levels branch from fd47a90 to 1070040 Compare March 17, 2022 16:54

github-actions bot added feature and removed feature labels Mar 17, 2022

MarvinJWendt reviewed Mar 18, 2022

View reviewed changes

Brookke force-pushed the issue-261-Add_ability_to_set_custom_io_writter_for_each_of_the_different_output_levels branch from 6dc4bf2 to 28fd8c1 Compare March 23, 2022 18:27

floaust and others added 6 commits March 23, 2022 18:48

feat: added a custom writer for all printers

c5d92b0

remove from area printer due to more complex requirements. SAdd to ba…

609ed85

…sic text printer

remove unused type

8c2b1f5

Use default outputter if none is provided

d8c2b90

rename to WithWriter

3024719

add basic tests

c50e99c

Brookke force-pushed the issue-261-Add_ability_to_set_custom_io_writter_for_each_of_the_different_output_levels branch from dc5ead7 to bcf821b Compare March 23, 2022 18:49

Brookke marked this pull request as ready for review March 23, 2022 18:49

confirm correct writer is used

48050e3

Brookke force-pushed the issue-261-Add_ability_to_set_custom_io_writter_for_each_of_the_different_output_levels branch from bcf821b to 48050e3 Compare March 23, 2022 18:50

Brookke requested a review from MarvinJWendt March 24, 2022 11:34

MarvinJWendt approved these changes Mar 28, 2022

View reviewed changes

MarvinJWendt merged commit dc10c49 into pterm:master Mar 28, 2022

MarvinJWendt changed the title ~~feat: added a custom writer for all printers~~ feat: added a custom writer for all text printers Mar 28, 2022

github-actions bot added feature and removed feature labels Mar 28, 2022

Brookke deleted the issue-261-Add_ability_to_set_custom_io_writter_for_each_of_the_different_output_levels branch March 28, 2022 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added a custom writer for all text printers #324

feat: added a custom writer for all text printers #324

Brookke commented Mar 17, 2022 •

edited

MarvinJWendt commented Mar 17, 2022

Brookke commented Mar 17, 2022

codecov bot commented Mar 17, 2022 •

edited

Brookke commented Mar 17, 2022 •

edited

Brookke commented Mar 17, 2022

MarvinJWendt commented Mar 17, 2022

MarvinJWendt commented Mar 18, 2022

MarvinJWendt left a comment

MarvinJWendt Mar 18, 2022

Brookke commented Mar 18, 2022

MarvinJWendt commented Mar 18, 2022

MarvinJWendt commented Mar 22, 2022 •

edited

Brookke commented Mar 23, 2022

Brookke commented Mar 23, 2022

MarvinJWendt left a comment

MarvinJWendt commented Mar 28, 2022

MarvinJWendt commented Mar 28, 2022

Brookke commented Mar 28, 2022

MarvinJWendt commented Mar 28, 2022

		// Print formats using the default formats for its operands and writes to standard output.
		// Print formats using the default formats for its operands and writes to provided writer.

feat: added a custom writer for all text printers #324

feat: added a custom writer for all text printers #324

Conversation

Brookke commented Mar 17, 2022 • edited

Description

Scope

Related Issue

To-Do Checklist

MarvinJWendt commented Mar 17, 2022

Brookke commented Mar 17, 2022

codecov bot commented Mar 17, 2022 • edited

Codecov Report

Brookke commented Mar 17, 2022 • edited

Brookke commented Mar 17, 2022

MarvinJWendt commented Mar 17, 2022

MarvinJWendt commented Mar 18, 2022

MarvinJWendt left a comment

Choose a reason for hiding this comment

MarvinJWendt Mar 18, 2022

Choose a reason for hiding this comment

Brookke commented Mar 18, 2022

MarvinJWendt commented Mar 18, 2022

MarvinJWendt commented Mar 22, 2022 • edited

Brookke commented Mar 23, 2022

Brookke commented Mar 23, 2022

MarvinJWendt left a comment

Choose a reason for hiding this comment

MarvinJWendt commented Mar 28, 2022

MarvinJWendt commented Mar 28, 2022

Brookke commented Mar 28, 2022

MarvinJWendt commented Mar 28, 2022

Brookke commented Mar 17, 2022 •

edited

codecov bot commented Mar 17, 2022 •

edited

Brookke commented Mar 17, 2022 •

edited

MarvinJWendt commented Mar 22, 2022 •

edited