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

refactor: Purge all println and ban it #253

Merged
merged 7 commits into from
Mar 1, 2022

Conversation

AndrewSisley
Copy link
Contributor

Closes #242

Commits are pretty broken down - more involved/controversial changes should have there own commit, with Delete println containing the benign.

Please speak out if you have concerns, this is very much a proposal (with my vote for it atm). Not a problem if this is rejected.

@AndrewSisley AndrewSisley added refactor This issue specific to or requires *notable* refactoring of existing codebases and components ci/build This is issue is about the build or CI system, and the administration of it. labels Mar 1, 2022
@AndrewSisley AndrewSisley self-assigned this Mar 1, 2022
@codecov
Copy link

codecov bot commented Mar 1, 2022

Codecov Report

Merging #253 (56d72e5) into develop (42a3b8b) will decrease coverage by 0.08%.
The diff coverage is 88.88%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #253      +/-   ##
===========================================
- Coverage    58.57%   58.49%   -0.09%     
===========================================
  Files          103      103              
  Lines         9846     9834      -12     
===========================================
- Hits          5767     5752      -15     
- Misses        3449     3454       +5     
+ Partials       630      628       -2     
Impacted Files Coverage Δ
api/http/api.go 0.00% <ø> (ø)
bench/bench_util.go 9.23% <0.00%> (ø)
bench/collection/utils.go 0.00% <ø> (ø)
bench/query/simple/utils.go 0.00% <ø> (ø)
db/collection_update.go 42.10% <ø> (+0.09%) ⬆️
db/fetcher/dag.go 56.81% <ø> (ø)
db/fetcher/fetcher.go 61.63% <ø> (-0.81%) ⬇️
db/schema.go 37.20% <ø> (-1.43%) ⬇️
db/db.go 53.57% <100.00%> (ø)
db/tests/utils.go 58.79% <100.00%> (-0.57%) ⬇️
... and 1 more

@orpheuslummis
Copy link
Contributor

This makes the benchmarks output a lot of logs if info log level is used, which is the current default.
Outside of that, LGTM.

@AndrewSisley
Copy link
Contributor Author

This makes the benchmarks output a lot of logs if info log level is used, which is the current default. Outside of that, LGTM.

It does? Cheers for checking - will have a look - thought that would be the same as before (maybe it is lol?)

Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

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

LGTM

@AndrewSisley
Copy link
Contributor Author

AndrewSisley commented Mar 1, 2022

This makes the benchmarks output a lot of logs if info log level is used, which is the current default. Outside of that, LGTM.

Is doing in dev branch - will merge this after creating a new ticket to fix the bench spam:
#260

@AndrewSisley AndrewSisley merged commit 38c4b2c into develop Mar 1, 2022
@AndrewSisley AndrewSisley deleted the sisley/refactor/I242-println-purge branch March 1, 2022 20:01
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
* Use logger in bench package

* Log command result instead of println

* Remove printlns from db_test

Here they are actually hidding what is actually untested

* Write to log in db.printstore

Instead of println

* Use logger in db.tests

* Delete println

* Ban fmt.PrintX
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/build This is issue is about the build or CI system, and the administration of it. refactor This issue specific to or requires *notable* refactoring of existing codebases and components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider banning fmt.Println* via linter rule
3 participants