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

No-Op Implementation #4

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

No-Op Implementation #4

wants to merge 11 commits into from

Conversation

rtsketo
Copy link

@rtsketo rtsketo commented Jul 13, 2020

Edit:

With this PR, two artifacts will be published, ktee and ktee-noop.
As the name suggests, ktee-noop has no internal functionality and it can be used in production environment with no overhead.

With this PR, the user can enable or disable logging and printing by setting KTee.debug variable true or false.

This can be automated depending your build, for example in Android the user can set the variable as KTee.debug = BuildConfig.DEBUG at the start of his app.

By default is set to true.

PS. I think this was suggested by someone in Reddit.

Copy link
Member

@kdabir kdabir left a comment

Choose a reason for hiding this comment

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

Thanks for taking time to contribute. At present we are not in position to merge this request because of two reasons. 1. it used mutable variable 2. it does not include test cases.

import org.slf4j.Logger

class KTee { companion object { var debug = true } }
Copy link
Member

Choose a reason for hiding this comment

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

we actively avoid using var in our codebase. This becomes a global variable which is not good in server/multithreaded/concurrent envs.

Copy link
Author

Choose a reason for hiding this comment

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

I've made some changes. The variable can now be set only once.
If you are concerned about thread safety, @volatile can also be used instead.
Other than that, I fail to see how it's possible to have a toggle without being a var.

@rtsketo
Copy link
Author

rtsketo commented Jul 16, 2020

Fixed the second issue.

@sonarcloud
Copy link

sonarcloud bot commented Jul 16, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@rtsketo rtsketo changed the title Toggable Printing and Logging. Toggable Output. Jul 17, 2020
@kdabir
Copy link
Member

kdabir commented Jul 21, 2020

Can we look at alternative like a No-Op interface just like slf4j. That way dependency can be swapped at build/compile time without having to change code. in no-op impl all the methods essentially do not output anything. We can produce two artifacts ktee and ktee-noop?

@rtsketo rtsketo changed the title Toggable Output. No-Op Implementation Dec 15, 2021
Aristos Pasalides added 3 commits December 15, 2021 23:37
This reverts commit 5c96e9b4

(cherry picked from commit 091249c)
(cherry picked from commit 876c111)
@sonarcloud
Copy link

sonarcloud bot commented Dec 15, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@rtsketo
Copy link
Author

rtsketo commented Dec 15, 2021

@kdabir, coming from the "Android world" where build types and flavors are a thing in Gradle, I can't be so sure I implemented the No-Op artifact publication in the best way possible. Please, let me know if there's something that can be improved.

@kdabir
Copy link
Member

kdabir commented Jun 6, 2022

apologies for the delay in responding. The changes are looking good. For no-op project, we could go with a multi-module approach rather than additional source set. However, I don't necessarily see problem with current approach too. Let me review this and provide more specific feedback. Thanks for incorporating the feedback.

@rtsketo
Copy link
Author

rtsketo commented Jul 3, 2022

Of course, I'm up for giving a try to any better solution. Could you please give me more info on how to approach a multi-module solution and what are the advantages compared to the current?

# Conflicts:
#	build.gradle
@sonarcloud
Copy link

sonarcloud bot commented Nov 19, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@rtsketo
Copy link
Author

rtsketo commented Nov 19, 2022

If someone is looking for this feature, I released a modified version here. Using a noop implementation in production does make reverse engineering slightly harder, thus I recommend it unconditionally in any non server-side code.

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.

2 participants