Code review comment for lp:~thomas-voss/trust-store/add_convenience_function_for_processing_incoming_requests

Revision history for this message
Thomas Voß (thomas-voss) wrote :

> 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"};"
>

Fixed.

> 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).
>

Fixed.

> 1530 + default:
> 1531 + break;
>
> Redundant "break;"

But the compiler needs it :)
>
> 2514 +std::shared_ptr<core::trust::Store::Query> a_null_query()
>

Keeping it for future convenience.

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

Removed.

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

Removed.

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

Removed.

> 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.

Fixed.

« Back to merge proposal