Skip to content
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

Move duplicate conversion functions to Message Helper #3449

Conversation

ychernysheva
Copy link
Contributor

Fixes #3448

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

Affected unit tests are updated

Summary

We have lots of similar conversion functions, which are located in different SDL places (MessageHelper, RAI request, Application Manager etc.). This conversion functions are duplicates of each other in terms of conversion from string to enum and vice versa. That's why there is a possibility to replace this functions/methods with unified template functions, located in MessageHelper, and delete redundant code or mark it as deprecated (in case public methods).

List of affected functions:

  • AppHMITypeToString/StringToAppHMIType
  • HMIResultFromString/HMIResultToString
  • MobileResultFromString/MobileResultToString
  • StringToHMILevel/StringifiedHMILevel
  • MobileLanguageToString/MobileLanguageFromString
  • CommonLanguageFromString/CommonLanguageToString
  • CommonLightNameFromString

CLA

@ychernysheva
Copy link
Contributor Author

@theresalech This PR is ready for review.
Thank you!

@theresalech
Copy link
Contributor

Hi @ychernysheva, issue 3448 is not currently planned for the Core 6.2 release, but we will work to include if time allows.

Copy link
Contributor

@jacobkeeler jacobkeeler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before I merge, you can deleted the deprecated methods since the next release is planned to be a major release.

@jacobkeeler
Copy link
Contributor

@ychernysheva also fix merge conflicts

@ychernysheva
Copy link
Contributor Author

@jacobkeeler All conflicts were resolved in 460daf4, deprecated methods were deleted in 3ae23f2, also actualization after merge of current develop was made in 663933d.

But due to unclear regression status on current develop branch (too many fails on 4c9f873) we cannot define regression status on branch with this fix. May I suggest to wait until develop becomes more stable to merge it and then check regression status?

@ychernysheva
Copy link
Contributor Author

@jacobkeeler Full regression check was passed successfully after merging of current develop branch.

@@ -0,0 +1,731 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this file added and why wasn't it needed before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file was added by mistake during resolving of merge conflicts. Now this file is absent on develop branch, so we don't need it. Deleted in d663395

@jacobkeeler
Copy link
Contributor

@ychernysheva unfortunately there seem to be new merge conflicts and there is a cppcheck issue with hmi_capabilities_test.cc:

$ cppcheck --force -isrc/3rd_party -isrc/3rd_party-static --quiet --error-exitcode=1 src
[src/components/application_manager/test/hmi_capabilities_test.cc:1488]: (error) Invalid number of character '{' when these macros are defined: ''.

@ychernysheva
Copy link
Contributor Author

@jacobkeeler Merge conflicts were resolved, also suggested change was implemented in 430a53f.
Full regression check is passed successfully.

@jacobkeeler jacobkeeler changed the base branch from develop to release/7.0.0 October 1, 2020 18:21
@jacobkeeler
Copy link
Contributor

jacobkeeler commented Oct 1, 2020

@LitvinenkoIra #3480 added another place where this will need to be done: https://github.com/smartdevicelink/sdl_core/blob/release/7.0.0/src/components/application_manager/src/resumption/resumption_data_processor_impl.cc#L52

Should be able to approve after that is addressed

@ychernysheva
Copy link
Contributor Author

@jacobkeeler Proposed change is implemented in 78a5677

@jacobkeeler jacobkeeler merged commit daf1d72 into smartdevicelink:release/7.0.0 Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants