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

Conversation

@drichelson
Copy link
Contributor

@drichelson drichelson commented May 28, 2016

V2 support. This code passes integration harness tests.
A big part of the changes you see are a rename of FeatureRep->FeatureFlag
TODO: in a separate PR: local tests for prereq cycle detection

}

dependencies {
compile "org.apache.httpcomponents:httpclient:4.3.6"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated all the deps to latest releases.

*
* @param key the key of the feature flag to evaluate to true.
*/
public void setFeatureTrue(String key) {
Copy link
Contributor Author

@drichelson drichelson May 28, 2016

Choose a reason for hiding this comment

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

I intended to keep the spirit of this class alive for true/false values, but renaming the on/off convention to true/false to avoid confusion with the fact that a flag can be on or off which may or may not yield true/false values.

private boolean on;
private List<Prerequisite> prerequisites;
private String salt;
private String sel;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can omit the sel since the server SDKs shouldn't need / see it.

@@ -0,0 +1,14 @@
package com.launchdarkly.client;

public class Prerequisite {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not be public, and should not have public members.

@jkodumal
Copy link
Contributor

jkodumal commented Jun 1, 2016

Looking pretty good. Some comments to address before I thumb, but it's definitely coming together!

@drichelson
Copy link
Contributor Author

@jkodumal if my recent commit looks good I'd like to merge this into the v2 branch as a wip- with this todo list:
-prereq cycle detection test
-changes so the integration harness passes.

@jkodumal
Copy link
Contributor

jkodumal commented Jun 2, 2016

As long as you have all my comments captured as tasks in Asana or Todoist, that's fine. There are a number of them that are required work, not just minor cleanup (e.g. the scope / visibility issues of the data model classes).

@drichelson
Copy link
Contributor Author

@jkodumal All your comments are addressed. The additional items are from me.

@jkodumal
Copy link
Contributor

jkodumal commented Jun 2, 2016

Yep, I see that now-- other than shading Joda time, which looks like it should still be on your list.

👍

@jkodumal
Copy link
Contributor

jkodumal commented Jun 2, 2016

BTW, if you're going to shade joda-time, I would also suggest shading gson. I think we've discovered empirical evidence that version conflicts can lead to incorrect behavior.

@drichelson drichelson merged commit a413b32 into v2 Jun 2, 2016
@drichelson drichelson deleted the dr/v2 branch June 2, 2016 16:29
eli-darkly added a commit that referenced this pull request Mar 26, 2018
fix error in javadoc code sample
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.

3 participants