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

StdlibAdapter: Make regexp configurable #33

Merged
merged 21 commits into from
Jun 6, 2023

Conversation

jkroepke
Copy link
Contributor

Potential solution for #32

The users knows, if the stdlog may contain a file key or not, since he has to configure the flag.

Fixes #32
Rel go-kit/kit#233

@ChrisHines
Copy link
Member

Thanks for the PR. I will try to review this in the next few days.

stdlib.go Show resolved Hide resolved
stdlib.go Outdated Show resolved Hide resolved
stdlib.go Outdated Show resolved Hide resolved
stdlib.go Outdated Show resolved Hide resolved
stdlib_test.go Outdated Show resolved Hide resolved
@jkroepke jkroepke changed the title StdlibAdapter: Make file key optional StdlibAdapter: Make regexp configurable May 13, 2023
@jkroepke jkroepke requested a review from ChrisHines May 13, 2023 08:27
stdlib.go Outdated Show resolved Hide resolved
stdlib.go Outdated Show resolved Hide resolved
stdlib.go Outdated Show resolved Hide resolved
stdlib.go Outdated Show resolved Hide resolved
stdlib.go Outdated Show resolved Hide resolved
stdlib.go Outdated Show resolved Hide resolved
stdlib.go Outdated Show resolved Hide resolved
stdlib.go Outdated Show resolved Hide resolved
stdlib.go Outdated Show resolved Hide resolved
@jkroepke
Copy link
Contributor Author

jkroepke commented May 14, 2023

I hope I address all issues for now. The breaking change may be discussable, however it would be the stdlib default.

I mark the regex and pattern exportable. The benefits are that users can re-use them instead do a copy and own.

stdlib.go Outdated Show resolved Hide resolved
stdlib_test.go Outdated Show resolved Hide resolved
stdlib_test.go Outdated Show resolved Hide resolved
stdlib_test.go Outdated Show resolved Hide resolved
stdlib_test.go Outdated Show resolved Hide resolved
stdlib.go Outdated Show resolved Hide resolved
stdlib_test.go Outdated Show resolved Hide resolved
stdlib_test.go Outdated Show resolved Hide resolved
stdlib_test.go Outdated Show resolved Hide resolved
stdlib.go Outdated Show resolved Hide resolved
stdlib.go Outdated Show resolved Hide resolved
Copy link
Member

@peterbourgon peterbourgon left a comment

Choose a reason for hiding this comment

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

Some nits, and I'd still like to see wider test coverage, but with these changes made, I'm basically OK, pending a thumbs-up from @ChrisHines

stdlib.go Outdated Show resolved Hide resolved
stdlib_test.go Outdated Show resolved Hide resolved
stdlib_test.go Outdated Show resolved Hide resolved
jkroepke and others added 16 commits May 27, 2023 23:41
Co-authored-by: Peter Bourgon <[email protected]>
Co-authored-by: Peter Bourgon <[email protected]>
Co-authored-by: Peter Bourgon <[email protected]>
Co-authored-by: Peter Bourgon <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
@coveralls
Copy link

coveralls commented May 27, 2023

Pull Request Test Coverage Report for Build 5191430246

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 11 of 12 (91.67%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.07%) to 74.667%

Changes Missing Coverage Covered Lines Changed/Added Lines %
stdlib.go 11 12 91.67%
Totals Coverage Status
Change from base Build 5100883045: 0.07%
Covered Lines: 560
Relevant Lines: 750

💛 - Coveralls

@ChrisHines
Copy link
Member

... I'm basically OK, pending a thumbs-up from @ChrisHines

I will review the latest updates soon.

Copy link
Member

@ChrisHines ChrisHines left a comment

Choose a reason for hiding this comment

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

Sorry for the delay reviewing this. It mostly looks great to me. Two nits and one big question.

stdlib.go Show resolved Hide resolved
stdlib.go Show resolved Hide resolved
stdlib.go Show resolved Hide resolved
stdlib_test.go Outdated Show resolved Hide resolved
Signed-off-by: Jan-Otto Kröpke <[email protected]>
@jkroepke jkroepke requested a review from ChrisHines June 6, 2023 06:18
@jkroepke
Copy link
Contributor Author

jkroepke commented Jun 6, 2023

I'm a bit tired here about providing a solution, feedback, suggestion provided, feedback, feedback from other maintainer, code changes again, feedback, suggestions, feedback. Getting const feedback from 2 maintainers feel like an endless loop here.

I really appreciate that you want the right, but this is way more time consuming compared to if you pushing an own code solution. Please continue, thanks!

@ChrisHines
Copy link
Member

I'm a bit tired here about providing a solution, feedback, suggestion provided, feedback, feedback from other maintainer, code changes again, feedback, suggestions, feedback. Getting const feedback from 2 maintainers feel like an endless loop here.

I really appreciate that you want the right, but this is way more time consuming compared to if you pushing an own code solution. Please continue, thanks!

I sympathize. It has been a long path here. The subtest names are my only real concern, but I am fine with leaving them as is if @peterbourgon agrees.

Fundamentally, though, these test names interfere with the conventions expected by the testing package as described here: https://pkg.go.dev/testing#hdr-Subtests_and_Sub_benchmarks. The /'s used in the test names are indistinguishable from the /'s used by testing as separators in the test hierarchy.

My comments about VS Code issues were meant just as an observation.

Signed-off-by: Jan-Otto Kröpke <[email protected]>
@jkroepke
Copy link
Contributor Author

jkroepke commented Jun 6, 2023

I give each test case a name for now. this should cover the issues.

@peterbourgon
Copy link
Member

peterbourgon commented Jun 6, 2023

Thanks for this contribution, and for enduring the gauntlet of review by the maintainers.

Working in Public: The Making and Maintenance of Open Source Software analyzes the dynamics of OSS projects such as go-kit/log, and arrives at a conclusion that I think is both extremely important, and not well-understood by many OSS participants. That conclusion is that pull requests are usually net negative to most open-source projects, because the value they provide is usually out-weighed by the maintenance burden they incur on the project maintainers. Consequently, it's critical that OSS projects have a very strict "barrier to entry" for arbitrary patches. In this case, that manifested as several rounds of review. I hope you can sympathize with the rationale: by accepting this PR, the maintainers are taking ownership of the code, into perpetuity.

In any case, it worked out 👍 so thank you.

@peterbourgon peterbourgon merged commit c7bf814 into go-kit:main Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NewStdlibAdapter: messages split wrongly, if it contains a colon
4 participants