[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