Skip to content

Feat/#309 read config from arbitrary directories#386

Merged
stevehu merged 20 commits intodevelopfrom
feat/#309-read-config-from-arbitrary-directories
Feb 14, 2019
Merged

Feat/#309 read config from arbitrary directories#386
stevehu merged 20 commits intodevelopfrom
feat/#309-read-config-from-arbitrary-directories

Conversation

@jiachen1120
Copy link
Copy Markdown
Contributor

@jiachen1120 jiachen1120 commented Feb 11, 2019

Related issue: #309
Related RFC: https://github.com/networknt/light-rfcs/blob/master/light-4j/0012-flexible-config-directory.md

Three new overloaded methods provided, in this PR, the path can only be the path relative to light-4j-config-dir.
public abstract Map<String, Object> getJsonMapConfig(String configName, String path);
public abstract Map<String, Object> getJsonMapConfigNoCache(String configName, String path);
public abstract Object getJsonObjectConfig(String configName, Class clazz, String path);

Extended ConfigDefaultTest to included the test for these methods. The ConfigDefaultTest could not work properly before if build the whole light-4j project entirely. Since the config is a singleton and shared by all classes, the instance of config would already exist before running ConfigDefaultTest. If we set the light-4j-config-dir directly, it cannot be injected into config instance. So the config in ConfigDefaultTest would still be the classpath one. So now I use reflection to inject field in the test.

Look forward to your review!

}
if (isBoolean(str)) {
return str.equals("true") || str.equals("True") || str.equals("TRUE") ? true : false;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can be simplified to just return str.equals("true") || str.equals("True") || str.equals("TRUE"); right?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

or
return str.toLowerCase().equals("true");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will simplify it. @miklish The reason why I didn't use toLowerCase() is to uniform with yaml rules, ie "tRue", "trUe", etc. should not be recognized as boolean. Do you think it makes sense?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It simply depends on how strict you want your parsing to be in this instance.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@jiachen1120 Agreed, we shouldn't use toLowerCase.
Actually, if you want to be even closer to the yaml boolean spec, we would use these:

 y|Y|yes|Yes|YES|n|N|no|No|NO
|true|True|TRUE|false|False|FALSE
|on|On|ON|off|Off|OFF

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will improve it to be more strict.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

And I will modify the light-doc as well.

@jiachen1120 jiachen1120 mentioned this pull request Feb 13, 2019
@stevehu stevehu merged commit d13f126 into develop Feb 14, 2019
@DSchrupert DSchrupert deleted the feat/#309-read-config-from-arbitrary-directories branch February 14, 2019 19:13
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.

6 participants