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

Update python.md #1582

Closed
wants to merge 1 commit into from
Closed

Update python.md #1582

wants to merge 1 commit into from

Conversation

inirudebwoy
Copy link

About the changes

Updating documentation to match what I have done here Unleash/unleash-client-python#207

@vercel
Copy link

vercel bot commented May 10, 2022

@inirudebwoy is attempting to deploy a commit to the unleash-team Team on Vercel.

A member of the Team first needs to authorize it.

@thomasheartman
Copy link
Contributor

Hey! Thanks for the PR 😄 I see that this is the same as in the file you linked, but I'm not sure I understand why. I'm probably missing some context, but why are you duplicating it? Using the fallback_function argument is already shown just a couple lines down.

@thomasheartman thomasheartman self-requested a review May 12, 2022 10:14
@inirudebwoy
Copy link
Author

Hey! Thanks for the PR 😄 I see that this is the same as in the file you linked, but I'm not sure I understand why. I'm probably missing some context, but why are you duplicating it? Using the fallback_function argument is already shown just a couple lines down.

Hey @thomasheartman
Those docs have same issue as ones I have fixed. default_value keyword argument will raise exception. Maybe the docs need a bit more than that. As you correctly pointed out fallback_function is couple lines below my change. The examples show different features though. They show "how to set default" and "how to add fallback function".

@thomasheartman
Copy link
Contributor

thomasheartman commented May 13, 2022

Thanks for the clarification, but I'm still not sure I understand completely. I'd be very happy to have this change in the docs too, but I just want to make sure I understand what's happening first, so that we can explain it to the users. And if the default_value keyword has stopped working, then that's something we should also clarify. When did that happen?

Those docs have same issue as ones I have fixed. default_value keyword argument will raise exception.

Does it? That sounds mighty strange. I couldn't find an issue that describes it. Was there an issue raised? In that case: could you link me to it, please? ☺️

The examples show different features though. They show "how to set default" and "how to add fallback function".

Sorry, I'm probably missing something, but the two snippets appear to be exactly the same. How are they showing two different features?

EDIT: I found this PR from 2020 🙋🏼 Is that the source of this change?

@ivarconr ivarconr requested a review from sighphyre June 21, 2022 07:04
@sighphyre
Copy link
Member

@thomasheartman Thanks for finding that PR, I was just looking for that! From what I can tell, yes, that's the source of the default value being removed. The exception being raised here when the default value parameter is passed is just how Python handles bad parameters in a function call

Copy link
Member

@sighphyre sighphyre left a comment

Choose a reason for hiding this comment

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

I agree with this in principle, I've provided a small suggestion on the change. It would also be nice if we could update the docs strings for this method in ./UnleashClient/init.py because that also references the old default value. I don't mind doing that in a separate PR though

def custom_fallback(feature_name: str, context: dict) -> bool:
return True

client.is_enabled("My Toggle", fallback_function=custom_fallback)
Copy link
Member

Choose a reason for hiding this comment

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

This isn't wrong but a lambda would point out that this is different from the previous call, as per @thomasheartman's comments and also be a bunch simpler which is always nice. Something like

unleash_client.is_enabled("ThisFlagDoesn'tExist", fallback_function=lambda: True)

What do you think?

@thomasheartman
Copy link
Contributor

@sighphyre Thanks for jumping in here! Just thought I'd put forth that I added similar changes to the usage.rst file a while back in Unleash/unleash-client-python#208. We could potentially just copy that if you think that's alright?

@sighphyre
Copy link
Member

@thomasheartman No worries! And yes that looks absolutely awesome, let's copy that!

@thomasheartman
Copy link
Contributor

@inirudebwoy Thanks again for making us aware of this and submitting a suggested fix 🙌🏼 I've made changes to the documentation in #1752 which should cover the cases you've described. I'm closing this PR in favor of the other one due to inactivity. If you feel that it doesn't address the issues you were having, please let us know and we'll rework the docs some more ☺️

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.

3 participants