Skip to content
This repository has been archived by the owner on May 30, 2024. It is now read-only.

Evaluate "interpreted" attributes when they are set as custom attributes. #52

Closed

Conversation

ghost
Copy link

@ghost ghost commented May 26, 2016

An interesting situation arose where if you don't set "interpreted" attributes via the special builder methods in LDUser they are not evaluated at all (e.g. when set via .custom()). I found this a little counterintuitive because it was not immediately obvious - or explicit that this was the case via the documentation / javadoc.

This change adds the ability to also evaluate them if a user sets an "interpreted" special attribute via custom. It implicitly also allows you to evaluate against a list of them like other custom attributes.

I am not sure if there was a deliberate reason for choosing to only make them evaluate if set via the dedicated methods. I noticed the same thing happens in other clients: https://github.com/launchdarkly/python-client/blob/master/ldclient/util.py#L69

If there is a good reason for not allowing this fallback - then I would suggest an update to documentation to more explicitly cover which fields are considered "interpreted" / "special" / "builtin" so it's less easy to fall into this situation.

@drichelson
Copy link
Contributor

Hi Ivor, Thanks for pointing this out. It is indeed deliberate that we only evaluate the 'interpreted' attributes when set by their special builder methods. The reason for this is that we want to be able to treat these fields with special meaning. I think the best way to move forward on this is to update the Javadoc for the custom() method indicating that any reserved attribute names will be ignored when setting them via the custom() builder method.
I'll submit a PR with this change shortly.

@drichelson
Copy link
Contributor

I've updated the Javadoc in #53. We'll close this PR in favor of that one

@drichelson drichelson closed this May 26, 2016
eli-darkly added a commit that referenced this pull request Feb 22, 2018
fix stream put handling - and add unit tests
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants