[WIP] Log marker #24

cyucelen · 2019-10-14T08:36:07Z

It stands to reason that one of the strongest use cases would be for users to colorize their logs for easy reading.

Build a abstraction structure upon Golang's logger to colorize. (We can experiment here!)
Implement io.Writer
Add Mark Rules (Mark Rule name can change)
- Builder way
- Passing Rule slice
README documentation

We can add further features to the checklist!

Closes #17

codecov-io · 2019-10-14T08:40:47Z

Codecov Report

Merging #24 into master will decrease coverage by 1.62%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
- Coverage     100%   98.37%   -1.63%     
==========================================
  Files           3        4       +1     
  Lines         106      123      +17     
==========================================
+ Hits          106      121      +15     
- Misses          0        1       +1     
- Partials        0        1       +1

Impacted Files	Coverage Δ
log.go	`88.23% <88.23%> (ø)`
matcher.go	`100% <0%> (ø)`	⬆️

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 03dc5bf...70b11f8. Read the comment docs.

log.go

cyucelen · 2019-10-17T20:30:26Z

Do we need to create a variadic AddRule method?
What do you think about its usefulness?

Darkclainer · 2019-10-17T20:56:41Z

I think it should be another PR. See issue #27.

cyucelen · 2019-10-17T21:54:32Z

What should be another PR?
Buffering writer output until \n?

jnatalzia · 2019-10-18T00:10:22Z

Re: Variadic method

For the first pass I feel like the slice approach is good enough but if it's not a huge ordeal we might as well put it in this PR. What do you think the LoE is?

jnatalzia

looking so good 👍 few questions/suggestions!

log.go

log_test.go

cyucelen · 2019-10-18T10:31:23Z

Oh, we need to test the rule apply order by the way. Since we are applying the rules sequentially, some matches can overlap - later order matcher will override the previous ones if there is an overlap.

Users need to be aware of it and add their mark rules according to the order too. We need to document this and give an example for it too.

cyucelen · 2019-10-18T10:34:17Z

Re: Variadic method

For the first pass I feel like the slice approach is good enough but if it's not a huge ordeal we might as well put it in this PR. What do you think the LoE is?

I don't know what LoE is 😄 but we can skip it for now, less is more!

cyucelen · 2019-10-18T11:32:32Z

Implemented some helper functionality for our assertions in assert.go and tested mark rules with it. Can you check it out? Then we can resolve the corresponding reviews.

Darkclainer · 2019-10-18T11:38:30Z

You can not compare function. See https://stackoverflow.com/questions/42140758/collection-of-unique-functions-in-go/42147285#42147285

cyucelen · 2019-10-18T11:43:59Z

You can not compare function. See https://stackoverflow.com/questions/42140758/collection-of-unique-functions-in-go/42147285#42147285

This approach actually looks like working. I tested it for our MatcherFunc type and it fails when MatcherFuncs actually not equal.

Darkclainer · 2019-10-18T12:18:40Z

This behaviour is implementation defined.

cyucelen · 2019-10-18T12:54:07Z

This behaviour is implementation defined.

You mean undefined?

Darkclainer · 2019-10-18T13:47:36Z

No. Current standard says that functions are incomparable, see https://golang.org/ref/spec#Comparison_operators .
Using reflect in that particular case, with current go implementation you got desired behavior.

cyucelen · 2019-10-18T14:00:13Z

I get it.

This behavior is not specified and so, can vary between different compiler implementations.
Here is the question; is it really a big problem for us?

If it is, then, is there any other option for directly testing the AddRules behaviour via asserting MarkRule equality. (You said something about wrapping it with interface?)
Or, we can test it indirectly in Write functions' tests, this is fine too. (I mean just testing the behavior of Write can cover AddRules logic too)

I prefer the second way if the tests for the first option will impact the implementation and make it more complex.

Darkclainer · 2019-10-18T15:49:21Z

I think second option is fine, but you should come up with test that cover order of rules too.

cyucelen · 2019-10-18T16:07:02Z

Aye aye, captain!

jnatalzia · 2019-10-21T19:56:42Z

log_test.go

+
+	expectedLog = fmt.Sprintf("best %s %s is %s\n", red("data"), red("company"), red(blue("skydome")))
+	logger.Print("best data company is skydome")
+	assert.Equal(t, expectedLog, mockOut.actualLog)


nice, this is so cool

jnatalzia · 2019-10-22T20:09:09Z

@cyucelen is it just the readme docs that remain for this feature? super excited to integrate it into some projects

cyucelen · 2019-10-22T20:19:45Z

@jnatalzia yes, can you help about docs?

jnatalzia · 2019-10-22T21:27:53Z

Can do!

jnatalzia · 2019-10-22T22:03:18Z

log.go

+}
+
+// NewWriteMarker creates a Marker that writes out to the given io.Writer
+func NewWriteMarker(writer io.Writer, options ...StdoutMarkerOption) *StdoutMarker {


hey @cyucelen in writing the docs, I refactored the way that we deal with custom outs. instead of having to replace stdout on the created marker, i flipped it so both cases (stdout and custom) require a single function call and no more than that.

let me know what you think, happy to discuss changing it back but it felt more natural when making the example file for this writer type so i thought it might be a good change

jnatalzia · 2019-10-22T22:03:43Z

Docs added! Also refactored a small portion of the code, added a comment explaining what and why to the PR

cyucelen · 2019-10-23T16:23:23Z

We merged the @Darkclainer s unified image PR to master. Can you integrate it to this branch? I will review your last commit about Writer asap!

jnatalzia · 2019-10-24T13:03:48Z

Yes I can! Also seems like I need to add some code coverage, gotta love CI. Will fix it up today at some point

…itation

codecov-io · 2019-10-25T12:41:54Z

Codecov Report

Merging #24 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #24   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      4    +1     
  Lines         106    121   +15     
=====================================
+ Hits          106    121   +15

Impacted Files	Coverage Δ
log.go	`100% <100%> (ø)`

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 03dc5bf...71e4d90. Read the comment docs.

jnatalzia · 2019-10-25T12:42:53Z

Hmmm @Darkclainer can we make an issue/PR to update the docs for using the image generator? It requires a pip install (maybe foreign for a go repo) as well as two separate terminal dependencies but no indication of where to get them. I can guess but given I'm installing them in the shell, I'd prefer to have sanctioned sources

cc @cyucelen

cyucelen · 2019-10-25T19:45:04Z

Awesome PR @jnatalzia!

I just added some docs for image_generator, also updated the images of log feature in readme.

Maybe we can get rid of functional options in WriteMarker since we are not using it anymore ?

Looks like we are getting ready for the deployment! 😎

Also, what do you think about #29 ? @jnatalzia @Darkclainer

jnatalzia · 2019-10-25T23:29:23Z

Maybe we can get rid of functional options in WriteMarker since we are not using it anymore ?

Sounds like a plan! Will push an update with those removed and then I think we're good to merge. Will mark as no longer draft.

Will comment on #29 on the issue!

cyucelen · 2019-10-26T09:15:03Z

Looks like we are good to go! Do the final touch, merge it!

cyucelen added enhancement New feature or request Hacktoberfest Festival of open source community! labels Oct 14, 2019

cyucelen self-assigned this Oct 14, 2019

jnatalzia reviewed Oct 15, 2019

View reviewed changes

log.go Outdated Show resolved Hide resolved

Darkclainer mentioned this pull request Oct 17, 2019

MarkWriter implementation #27

Open

jnatalzia reviewed Oct 18, 2019

View reviewed changes

log.go Show resolved Hide resolved

log_test.go Outdated Show resolved Hide resolved

log_test.go Outdated Show resolved Hide resolved

jnatalzia reviewed Oct 21, 2019

View reviewed changes

jnatalzia reviewed Oct 22, 2019

View reviewed changes

cyucelen added 8 commits October 25, 2019 05:37

started to implement logger

a26579e

print with multiple rules implemented

e99366b

log marker naming changes

5ca0006

AddRules implemented

703339d

io.Writer interface implemented instead of wrapping all logger methods

fb93fa5

comments added to StdoutMarker

2d4f1bc

new assertion helpers implemented for asserting MarkRule equality

def12fb

add rule behaviour tested in write because of function comparison lim…

8cbd258

…itation

jnatalzia force-pushed the log-marker branch from f301ff3 to 70b11f8 Compare October 25, 2019 12:37

Add documentation, refactor custom target for io writer

282cf53

jnatalzia force-pushed the log-marker branch from 70b11f8 to 282cf53 Compare October 25, 2019 12:49

cyucelen added 2 commits October 25, 2019 22:04

Readme added to image_generator

2ddfb3d

readme tidy up, log terminal images updated

082385f

Remove options arg as no longer needed

71e4d90

jnatalzia marked this pull request as ready for review October 25, 2019 23:31

jnatalzia merged commit 76d12a1 into master Oct 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Log marker #24

[WIP] Log marker #24

cyucelen commented Oct 14, 2019 •

edited

Loading

codecov-io commented Oct 14, 2019 •

edited

Loading

cyucelen commented Oct 17, 2019

Darkclainer commented Oct 17, 2019

cyucelen commented Oct 17, 2019

jnatalzia commented Oct 18, 2019

jnatalzia left a comment

cyucelen commented Oct 18, 2019

cyucelen commented Oct 18, 2019

cyucelen commented Oct 18, 2019

Darkclainer commented Oct 18, 2019

cyucelen commented Oct 18, 2019

Darkclainer commented Oct 18, 2019

cyucelen commented Oct 18, 2019

Darkclainer commented Oct 18, 2019

cyucelen commented Oct 18, 2019

Darkclainer commented Oct 18, 2019

cyucelen commented Oct 18, 2019

jnatalzia Oct 21, 2019

jnatalzia commented Oct 22, 2019

cyucelen commented Oct 22, 2019

jnatalzia commented Oct 22, 2019

jnatalzia Oct 22, 2019

jnatalzia commented Oct 22, 2019

cyucelen commented Oct 23, 2019

jnatalzia commented Oct 24, 2019

codecov-io commented Oct 25, 2019 •

edited

Loading

jnatalzia commented Oct 25, 2019

cyucelen commented Oct 25, 2019

jnatalzia commented Oct 25, 2019

cyucelen commented Oct 26, 2019

[WIP] Log marker #24

[WIP] Log marker #24

Conversation

cyucelen commented Oct 14, 2019 • edited Loading

codecov-io commented Oct 14, 2019 • edited Loading

Codecov Report

cyucelen commented Oct 17, 2019

Darkclainer commented Oct 17, 2019

cyucelen commented Oct 17, 2019

jnatalzia commented Oct 18, 2019

jnatalzia left a comment

Choose a reason for hiding this comment

cyucelen commented Oct 18, 2019

cyucelen commented Oct 18, 2019

cyucelen commented Oct 18, 2019

Darkclainer commented Oct 18, 2019

cyucelen commented Oct 18, 2019

Darkclainer commented Oct 18, 2019

cyucelen commented Oct 18, 2019

Darkclainer commented Oct 18, 2019

cyucelen commented Oct 18, 2019

Darkclainer commented Oct 18, 2019

cyucelen commented Oct 18, 2019

jnatalzia Oct 21, 2019

Choose a reason for hiding this comment

jnatalzia commented Oct 22, 2019

cyucelen commented Oct 22, 2019

jnatalzia commented Oct 22, 2019

jnatalzia Oct 22, 2019

Choose a reason for hiding this comment

jnatalzia commented Oct 22, 2019

cyucelen commented Oct 23, 2019

jnatalzia commented Oct 24, 2019

codecov-io commented Oct 25, 2019 • edited Loading

Codecov Report

jnatalzia commented Oct 25, 2019

cyucelen commented Oct 25, 2019

jnatalzia commented Oct 25, 2019

cyucelen commented Oct 26, 2019

cyucelen commented Oct 14, 2019 •

edited

Loading

codecov-io commented Oct 14, 2019 •

edited

Loading

codecov-io commented Oct 25, 2019 •

edited

Loading