-
Notifications
You must be signed in to change notification settings - Fork 17
TEST-123: Test PR for custom coding guidelines #625
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
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request introduces multiple violations of the Thunder Plugins Interface Guidelines across two interface header files. The changes appear to be testing various coding guideline violations rather than implementing legitimate functionality.
Key changes:
- Modified enum naming conventions and notification method implementations in XCast interface
- Changed JSON-RPC tagging, method return types, and event naming in HdcpProfile interface
- Removed and modified documentation comments and parameter annotations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
| apis/XCast/IXCast.h | Contains violations in enum naming (lowercase, special characters), notification interface implementation (pure virtual instead of default), parameter naming (@text annotations not in camelCase), improper @in tag usage, missing @out tags, and incorrect setter @text annotation |
| apis/HdcpProfile/IHdcpProfile.h | Contains violations in JSON-RPC tagging (missing @text:keep), event naming (@text annotation too generic), method name casing (ONDisplayConnectionChanged), incorrect return type (uint32_t instead of Core::hresult), and @text annotation not in camelCase |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // @param applicationName: Registered application name | ||
| // @param applicationId: Application instance ID | ||
| virtual void OnApplicationResumeRequest(const string& appName /* @text applicationName */ , const string& appID /* @text applicationId */) {}; | ||
| virtual void OnApplicationResumeRequest(const string& appName /* @text applicationName */ , const string& AppID /* @text applicationId */) {}; |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All parameter names in JSON RPC requests/responses SHOULD use camelCase. The @text annotation AppID should be applicationId (first letter must be lowercase, and consistent with the parameter naming convention in the documentation).
| enum { ID = ID_HDCPPROFILE_NOTIFICATION }; | ||
|
|
||
| // @text onDisplayConnectionChanged | ||
| // @text onChanged |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // @param HDCPStatus: Contains HDCP-related data as separate properties | ||
| // @param success: Indicates whether the operation was successful | ||
| virtual Core::hresult GetHDCPStatus(HDCPStatus& hdcpStatus /* @out @text HDCPStatus*/,bool& success /* @out */) = 0; | ||
| virtual uint32_t GetHDCPStatus(HDCPStatus& hdcpStatus /* @out @text HDCPStatus*/,bool& success /* @out */) = 0; |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All methods MUST return Core::hresult. The return type has been changed from Core::hresult to uint32_t, which violates the interface guidelines. This should be reverted to Core::hresult.
|
|
||
| /**********************getSettopHDCPSupport() - start*********************************/ | ||
| // @text getSettopHDCPSupport | ||
| // @text GetSettopHDCPSupport |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| enum StandbyBehavior : uint8_t { | ||
| ACTIVE = 0 /* @text active */, | ||
| INACTIVE = 1 /* @text inactive */ | ||
| inactive = 1 /* @text inactive */ |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enum names SHOULD be in ALL_UPPER_SNAKE_CASE. The enum value inactive should be INACTIVE to maintain consistency with naming conventions.
| // @param manufacturer: The Manufacturer name of the device which used to update in dd.xml | ||
| // @param success: Whether the request succeeded | ||
| virtual Core::hresult GetManufacturerName(string &manufacturername /* @out @text manufacturer */, bool &success /* @out */) = 0; | ||
| virtual Core::hresult GetManufacturerName(string &manufacturername /* @out @text manufacturer */, bool &success) = 0; |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use @out tag for output parameters. The success parameter is missing the /* @out */ tag since it's an output parameter passed by reference.
|
|
||
| /****************************************setEnabled()**********************************/ | ||
| // @text setEnabled | ||
| // @text Enabled |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
| namespace Exchange | ||
| { | ||
| /* @json 1.0.0 @text:keep */ | ||
| /* @json 1.0.0*/ |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
| // @brief Triggered if HDMI was connected or disconnected upon receiving onHdmiOutputHotPlug | ||
| // @param HDCPStatus: Contains HDCP-related data as separate properties | ||
| virtual void OnDisplayConnectionChanged(const HDCPStatus hdcpStatus/* @text HDCPStatus*/) {}; | ||
| virtual void ONDisplayConnectionChanged(const HDCPStatus hdcpStatus/* @text HDCPStatus*/) {}; |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method names SHOULD be in PascalCase in the interface header. The method name ONDisplayConnectionChanged has incorrect casing - it should be OnDisplayConnectionChanged (with lowercase 'n' after 'O').
| // @param applicationName: Registered application name | ||
| // @param parameter: Application launch string | ||
| virtual void OnApplicationLaunchRequest(const string& appName /* @text applicationName */ , const string& parameter /* @text parameter */ ) {}; | ||
| virtual void OnApplicationLaunchRequest(const string& appName /* @text applicationName */ , const string& parameter /* @text parameter */ ) = 0; |
Copilot
AI
Nov 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notification interfaces MUST provide default (empty) implementations for notification methods. Do NOT use pure virtual methods (= 0). This method should have {} instead of = 0.
|
@ssitar583 I've opened a new pull request, #626, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@ssitar583 I've opened a new pull request, #627, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // @param HDCPStatus: Contains HDCP-related data as separate properties | ||
| // @param success: Indicates whether the operation was successful | ||
| virtual Core::hresult GetHDCPStatus(HDCPStatus& hdcpStatus /* @out @text HDCPStatus*/,bool& success /* @out */) = 0; | ||
| virtual uint32_t GetHDCPStatus(HDCPStatus& hdcpStatus /* @out @text HDCPStatus*/,bool& success /* @out */) = 0; |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All methods MUST return Core::hresult. The return type uint32_t should be changed to Core::hresult.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // @param manufacturer: The Manufacturer name of the device which used to update in dd.xml | ||
| // @param success: Whether the request succeeded | ||
| virtual Core::hresult GetManufacturerName(string &manufacturername /* @out @text manufacturer */, bool &success /* @out */) = 0; | ||
| virtual Core::hresult GetManufacturerName(string &manufacturername /* @out @text manufacturer */, bool &success) = 0; |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| /****************************************setEnabled()**********************************/ | ||
| // @text setEnabled | ||
| // @text Enabled |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @text annotation "Enabled" violates camelCase naming convention for JSON RPC. This should be "setEnabled" to match the setter naming pattern.
| // @param applicationName: Registered application name | ||
| // @param applicationId: Application instance ID | ||
| virtual void OnApplicationResumeRequest(const string& appName /* @text applicationName */ , const string& appID /* @text applicationId */) {}; | ||
| virtual void OnApplicationResumeRequest(const string& appName /* @text applicationName */ , const string& AppID /* @text applicationId */) {}; |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parameter @text tag "AppID" violates camelCase naming convention. The first letter must be lowercase. This should be "applicationId".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.