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

Use of default value for for properties using the Key Expansion mechanism #84

Merged
merged 2 commits into from Jul 24, 2014

Conversation

@andreipet
Copy link
Contributor

@andreipet andreipet commented Jul 6, 2014

We have to get default values even for properties using the Key Expansion mechanism.
I see 2 alternatives:

  1. We have to try get the value for unexpanded key when the expanded key is not defined in source.
  2. Fill the properties manager with default values for all expanded keys, then add values found in source.

I had chosen the first solution.

andreipet added 2 commits Jul 6, 2014
We try to find the unexpanded key in propertiesManager to locate default value when the expandedKey is not found.
We want to obtain default value if anything goes wrong in locating expanded key in our source.
@coveralls
Copy link

@coveralls coveralls commented Jul 6, 2014

Coverage Status

Coverage increased (+0.01%) when pulling 9ee32ee on andreipet:master into ddfdfc0 on lviggiano:master.

@lviggiano
Copy link
Owner

@lviggiano lviggiano commented Jul 8, 2014

Thanks. I'll look into it asap.

Luigi.

@lviggiano
Copy link
Owner

@lviggiano lviggiano commented Jul 8, 2014

The unit test looks correct, so the use case you spotted should work as you expect. But I'll try to get a more linear implementation, based on your commit, before merging. Thanks.

@lviggiano
Copy link
Owner

@lviggiano lviggiano commented Jul 15, 2014

Hi @andreipet,

I've checked your code, but I need some more time to check the consistency of other features. Give me some more time, in this period I'm busy with work and personal life.

Thank you again for your contribution.

L.

@lviggiano lviggiano added bug labels Jul 15, 2014
@andreipet
Copy link
Contributor Author

@andreipet andreipet commented Jul 15, 2014

Sure man. Take your time. Thank you for your reply.
On Jul 15, 2014 4:01 PM, "Luigi R. Viggiano" notifications@github.com
wrote:

Hi @andreipet,

I've checked your code, but I need some more time to check the
consistency of other features. Give me some more time, in this period I'm
busy with work and personal life.

Thank you again for your contribution.

L.


Reply to this email directly or view it on GitHub.

@andreipet
Copy link
Contributor Author

@andreipet andreipet commented Jul 15, 2014

By the way. When you will have time, we could talk about performance.
I've started recently to develop in Java. But looking in owner code, I
found a way to deliver properties 10x times faster at least. Yeap , cache
works for read only, or even writable properties. CU
On 15 Jul 2014 17:21, "Andrei Petrovici" andrei.petrovici@gmail.com wrote:

Sure man. Take your time. Thank you for your reply.
On Jul 15, 2014 4:01 PM, "Luigi R. Viggiano" notifications@github.com
wrote:

Hi @andreipet,

I've checked your code, but I need some more time to check the
consistency of other features. Give me some more time, in this period I'm
busy with work and personal life.

Thank you again for your contribution.

L.


Reply to this email directly or view it on GitHub.

@lviggiano
Copy link
Owner

@lviggiano lviggiano commented Jul 15, 2014

I invite you to open an improvement request and explain the code spot where you found the performance bottlenecks.

What I learned about performance tuning is: first you profile, then you optimize: http://c2.com/cgi/wiki?ProfileBeforeOptimizing

Understanding performance hot spot just by looking at the code, is 99% of the times deceiving.

lviggiano added a commit that referenced this pull request Jul 24, 2014
Use of default value for for properties using the Key Expansion mechanism.
To be revised.
@lviggiano lviggiano merged commit 8ff32ab into lviggiano:master Jul 24, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
lviggiano added a commit that referenced this pull request Jul 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.