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

Adding disabled? method #130

Merged
merged 1 commit into from
Jan 5, 2023

Conversation

serhii-bodnaruk
Copy link
Contributor

About the changes

Adding disabled? method to more comfort using Unleash.

lib/unleash/client.rb Outdated Show resolved Hide resolved
lib/unleash/client.rb Outdated Show resolved Hide resolved
@serhii-bodnaruk serhii-bodnaruk force-pushed the add-disabled-method branch 2 times, most recently from e2a2d6f to f068ad7 Compare December 27, 2022 15:21
@coveralls
Copy link

coveralls commented Dec 27, 2022

Pull Request Test Coverage Report for Build 3836859355

  • 5 of 6 (83.33%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+96.5%) to 96.514%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/unleash/client.rb 4 5 80.0%
Totals Coverage Status
Change from base Build 3781944398: 96.5%
Covered Lines: 2132
Relevant Lines: 2209

💛 - Coveralls

@serhii-bodnaruk
Copy link
Contributor Author

@sighphyre could you take a look?

@rarruda
Copy link
Collaborator

rarruda commented Dec 29, 2022

@sighphyre @gardleopard

Opinions?

The things I can think of:

  • not a method in the other SDKs (?!), so it would make the ruby SDK a bit off compared with the API in the other SDKs. (Looked at node, Java, and python SDKs)
  • Maybe the recommended solution instead of if client.disabled?("foo") would be to use the unless keyword instead unless client.enabled?("foo") (?!)
  • method should be called is_disabled? with missing aliased method disabled? instead, for the sake of consistency.
  • Missing opposite test, for completeness sake. (That the disabled is the opposite in both true and false scenarios).
  • Missing docs in README (method description).

Wdyt?

@serhii-bodnaruk
Copy link
Contributor Author

serhii-bodnaruk commented Dec 29, 2022

@rarruda
It's not looking good and well readable "unless" staff, we prefer not to use it in simple cases.
From the beginning I added the method is_disabled? but your CI and rubocop setup doesn't like it, so renamed the method and remove the alias.

I can add more tests and README, my bad. But in case you guys like the idea in general. If not we will patch it on our end, no problem. 🙏

@rarruda
Copy link
Collaborator

rarruda commented Dec 29, 2022

Those were my 2c/thoughts.

Let's wait for what @sighphyre and @gardleopard think/decide.

But maybe @ivarconr would want to weigh in too?

Copy link
Contributor

@gardleopard gardleopard left a comment

Choose a reason for hiding this comment

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

Hi, this looks good! Thank you for your contribution. We would like to see some updates in the README.md and then its good:)

@serhii-bodnaruk serhii-bodnaruk requested review from gardleopard and removed request for gardleopard January 4, 2023 13:45
Copy link
Contributor

@gardleopard gardleopard left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for your contribution!

@gardleopard gardleopard merged commit f28859b into Unleash:main Jan 5, 2023
@serhii-bodnaruk serhii-bodnaruk deleted the add-disabled-method branch January 5, 2023 10:47
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.

4 participants