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

Add helper to modify Env configs (RIPD-1247) #2003

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
5 participants
@mellery451
Contributor

mellery451 commented Feb 7, 2017

Add envconfig test helper for manipulating Env config via
callables. Create new common modifiers for non-admin config,
validator config and one for using different server port values.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Feb 8, 2017

Codecov Report

Merging #2003 into develop will decrease coverage by <.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2003      +/-   ##
===========================================
- Coverage    67.66%   67.65%   -0.01%     
===========================================
  Files          680      680              
  Lines        49199    49204       +5     
===========================================
- Hits         33289    33288       -1     
- Misses       15910    15916       +6
Impacted Files Coverage Δ
src/ripple/app/ledger/PendingSaves.h 93.54% <0%> (-6.46%)
src/ripple/app/misc/impl/LoadFeeTrack.cpp 81.53% <0%> (-4.62%)
src/ripple/app/misc/ValidatorList.h 96.61% <0%> (-1.7%)
src/ripple/app/main/LoadManager.cpp 72.6% <0%> (-1.37%)
src/ripple/core/impl/Workers.cpp 98.86% <0%> (-1.14%)
src/ripple/core/impl/JobQueue.cpp 86.11% <0%> (-0.47%)
src/ripple/server/impl/BaseWSPeer.h 78.33% <0%> (+0.83%)
src/ripple/app/misc/impl/ValidatorList.cpp 83.33% <0%> (+1.46%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69bc58c...b7129b8. Read the comment docs.

codecov-io commented Feb 8, 2017

Codecov Report

Merging #2003 into develop will decrease coverage by <.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2003      +/-   ##
===========================================
- Coverage    67.66%   67.65%   -0.01%     
===========================================
  Files          680      680              
  Lines        49199    49204       +5     
===========================================
- Hits         33289    33288       -1     
- Misses       15910    15916       +6
Impacted Files Coverage Δ
src/ripple/app/ledger/PendingSaves.h 93.54% <0%> (-6.46%)
src/ripple/app/misc/impl/LoadFeeTrack.cpp 81.53% <0%> (-4.62%)
src/ripple/app/misc/ValidatorList.h 96.61% <0%> (-1.7%)
src/ripple/app/main/LoadManager.cpp 72.6% <0%> (-1.37%)
src/ripple/core/impl/Workers.cpp 98.86% <0%> (-1.14%)
src/ripple/core/impl/JobQueue.cpp 86.11% <0%> (-0.47%)
src/ripple/server/impl/BaseWSPeer.h 78.33% <0%> (+0.83%)
src/ripple/app/misc/impl/ValidatorList.cpp 83.33% <0%> (+1.46%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69bc58c...b7129b8. Read the comment docs.

@seelabs

I like how this turned out. Nice job. I left three nits, other than that looks good to me.

Show outdated Hide outdated src/test/rpc/ServerInfo_test.cpp Outdated
{
auto p = std::make_unique<Config>();
setupConfigForUnitTests(*p);
return p;

This comment has been minimized.

@seelabs

seelabs Feb 8, 2017

Contributor

How about changing modfunc so it takes the unique_ptr as a parameter and returns a unique_ptr? (we could also rename it to f). So the whole function would be: return f(envconf(), std::forward<Args>(args)...);

@seelabs

seelabs Feb 8, 2017

Contributor

How about changing modfunc so it takes the unique_ptr as a parameter and returns a unique_ptr? (we could also rename it to f). So the whole function would be: return f(envconf(), std::forward<Args>(args)...);

This comment has been minimized.

@mellery451

mellery451 Feb 8, 2017

Contributor

modfunc taking unique_ptr implies transfer of ownership in and then back out? or would it take unique_ptr const&? In any event, I think the semantics of modfunc should be modify but not own.

@mellery451

mellery451 Feb 8, 2017

Contributor

modfunc taking unique_ptr implies transfer of ownership in and then back out? or would it take unique_ptr const&? In any event, I think the semantics of modfunc should be modify but not own.

This comment has been minimized.

@seelabs

seelabs Feb 8, 2017

Contributor

Yes, I'd have it transfer ownership and then back out. All of these mod functions are meant to be used with envconf (if that wasn't the case, then passing the unique_ptr would be a bad interface). However, I don't see a scenario where they will be reused for anything other than envconf.

Pros:

  • This would allow a function to create a completely new config if it desires
  • The interface is clearer to me when modifications are returned rather than modifying a parameter (I dislike in/out parameters).

While I'll defend my POV, this is a minor issue. If you like the in/out parameter better it's your call.

@seelabs

seelabs Feb 8, 2017

Contributor

Yes, I'd have it transfer ownership and then back out. All of these mod functions are meant to be used with envconf (if that wasn't the case, then passing the unique_ptr would be a bad interface). However, I don't see a scenario where they will be reused for anything other than envconf.

Pros:

  • This would allow a function to create a completely new config if it desires
  • The interface is clearer to me when modifications are returned rather than modifying a parameter (I dislike in/out parameters).

While I'll defend my POV, this is a minor issue. If you like the in/out parameter better it's your call.

Show outdated Hide outdated src/test/jtx/envconfig.h Outdated
@seelabs

seelabs approved these changes Feb 9, 2017

👍

Show outdated Hide outdated src/test/app/Regression_test.cpp Outdated
@mellery451

This comment has been minimized.

Show comment
Hide comment
@mellery451

mellery451 Mar 1, 2017

Contributor

this has conflicts with some other passed PRs - will rebase and resolve after the next beta release

Contributor

mellery451 commented Mar 1, 2017

this has conflicts with some other passed PRs - will rebase and resolve after the next beta release

Add helper to modify Env configs (RIPD-1247)
Add envconfig test helper for manipulating Env config via
callables. Create new common modifiers for non-admin config,
validator config and one for using different server port values.

@mellery451 mellery451 added the Passed label Mar 14, 2017

@scottschurr scottschurr referenced this pull request Mar 21, 2017

Merged

Develop next #2059

@scottschurr

This comment has been minimized.

Show comment
Hide comment
@scottschurr

scottschurr Mar 21, 2017

Contributor

Merged to develop as 80d9b04

Contributor

scottschurr commented Mar 21, 2017

Merged to develop as 80d9b04

@mellery451 mellery451 deleted the mellery451:config_helpers branch Mar 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment