[Merge] lp:~thomas-voss/trust-store/add_convenience_function_for_processing_incoming_requests into lp:trust-store

Marcus Tomlinson marcus.tomlinson at canonical.com
Thu Jul 17 14:46:09 UTC 2014


Review: Needs Fixing

So I think this all looks pretty good, here are just a few more observations:

565	+        static std::string s{"com.ubuntu.trust.store"};

I know it doesn't really matter but the "const" is missing here.
Should be: "static const std::string s{"com.ubuntu.trust.store"};"

1493	+            (core::trust::mir::cli::option_title, boost::program_options::value<std::string>(), "Title of the prompt");

Missing a full-stop at the end of the string (doesn't match the others).

1530	+        default:
1531	+            break;

Redundant "break;"

2514	+std::shared_ptr<core::trust::Store::Query> a_null_query()

2052	+std::function<core::trust::Request::Answer(const core::posix::wait::Result&)> mock_translator_to_functor(const std::shared_ptr<MockTranslator>& ptr)

2062	+std::shared_ptr<core::trust::mir::PromptProviderHelper> a_prompt_provider_calling_bin_false()

2080	+std::shared_ptr<core::trust::mir::PromptProviderHelper> a_prompt_provider_calling_bin_true()

The above methods don't appear in your tests.

1426	=== added file 'src/core/trust/mir/prompt_main.cpp'
2786	=== added file 'tests/test_prompt.cpp'

These 2 files are pretty much doing the same thing. I get how one is a test version of the other but is it not a maintenance nightmare waiting to happen? If the app is used for internal use only, why not just check for something like a --testargs argument when running testing.
-- 
https://code.launchpad.net/~thomas-voss/trust-store/add_convenience_function_for_processing_incoming_requests/+merge/226696
Your team Ubuntu Phablet Team is subscribed to branch lp:trust-store.



More information about the Ubuntu-reviews mailing list