Implement ConfigurationJsonParsingStrategy#634
Conversation
52e85dc to
80fe517
Compare
castler
left a comment
castler
left a comment
There was a problem hiding this comment.
What would help to review this PR way more easy is todo the changes in a different way.
Meaning, instead of copying the code from one file to the other, to move the file into another file name (move config_parser -> configuration_json_parsing_strategy) in one commit and then perform the necessary changes on top of the file in another commit.
This should IMHO also reduce the changed lines significantly
| IConfigurationParsingStrategy& operator=(const IConfigurationParsingStrategy&) = default; | ||
| IConfigurationParsingStrategy& operator=(IConfigurationParsingStrategy&&) noexcept = default; | ||
|
|
||
| public: |
There was a problem hiding this comment.
We normally try to group the same visibilities together
| // See Note 1 | ||
| // coverity[autosar_cpp14_a15_5_3_violation] | ||
| auto ParseServiceTypeDeployment(const score::json::Object& json_map) -> ServiceTypeDeployment | ||
| auto Parse(const std::string_view path) -> Configuration |
There was a problem hiding this comment.
Parse will and shall not alter the state of the strategy, thus we shall mark it as const
| } | ||
| } | ||
| return ServiceTypeDeployment{score::cpp::blank{}}; | ||
| ConfigurationJsonParsingStrategy strategy; |
There was a problem hiding this comment.
We generally try to always use {}-initialization, just to be on the safe side.
e77a26e to
61e0b3f
Compare
5860927 to
eea0ec0
Compare
As much annoying it's, I did rename many times, but still in the full PR view it doesn't recognize that. |
b36dd6c to
91d01a1
Compare
11b2040 to
70b51f9
Compare
castler
left a comment
castler
left a comment
There was a problem hiding this comment.
Thanks for working on this - I will accept the PR, but just for future references:
1st) It would be nice to try to format the code in the right commit (and not change formatting in one commit and revert that in another)
2nd) More descriptive commit messages would be great. It will help us to generate better release notes, it will help others to understand why this commit was done, and it will help debugging in the future. For example one could write:
Wrap `ConfigurationParserStrategy` in free functions for ABI stability
With this commit we work towards our refactoring to enable configuration
from JSON and Flatbuffers. In a previous commit we move the JSON configuration
into a strategy, and in this commit we restore the previous API by a wrapper, that adds
no additional logic otherwise. This wrapper will be used in future to decide, if JSON or Flatbuffers configuration parsing shall be used.
Don't get me wrong - I also don't do this always - and often its just very short - but that would help a lot :)
| } | ||
| } // namespace | ||
| } // namespace score::mw::com::impl | ||
| } // namespace score::mw::com::impl No newline at end of file |
There was a problem hiding this comment.
its quite interesting to see that always the last line of the file is removed - I thought these kind of changes should be covered by our clang-format....
There was a problem hiding this comment.
"should be covered by our clang-format"
You mean during pushing as an adhoc in Github? or you mean locally?
| { | ||
| RecordProperty("Description", | ||
| "Checks that the free Parse(path) function delegates to ConfigurationJsonParsingStrategy."); | ||
| RecordProperty("TestType", "Requirements-based test"); |
There was a problem hiding this comment.
you do not link any requirement here?
I agree with you. |
github-project-automation
Bot
moved this from Backlog
to Done
in COM - Communication FT
2 participants
Uh oh!
There was an error while loading. Please reload this page.