Conversation
Documentation Auto-GeneratedDocumentation has been automatically generated for the changed plugins and committed to this PR branch. The generated documentation files have been added to the PR for review. |
There was a problem hiding this comment.
Pull request overview
Adds new Thunder Exchange interfaces for Account and Backup plugins and publishes generated API reference docs for them.
Changes:
- Introduced
IAccountinterface and generateddocs/apis/Account.md. - Introduced
IBackupManager/IBackupProviderinterfaces and generateddocs/apis/Backup.md. - Linked the new API docs into
docs/_sidebar.md.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/apis/Backup.md | Adds generated JSON-RPC documentation for Backup plugin methods. |
| docs/apis/Account.md | Adds generated JSON-RPC documentation for Account plugin methods. |
| docs/_sidebar.md | Adds navigation links for the new Account and Backup API docs. |
| apis/Backup/IBackup.h | Defines the Exchange interfaces for system backup/restore plus per-plugin backup/restore provider. |
| apis/Account/IAccount.h | Defines the Exchange interface for account-level operations. |
apis/Backup/IBackup.h
Outdated
| // @param scenario: Scenario for which the backup to happen | ||
| virtual Core::hresult Restore(const Scenario scenario) const = 0; | ||
|
|
||
| }; |
There was a problem hiding this comment.
Two issues in this block: (1) Restore's @param scenario description says 'backup' but should say 'restore'. (2) There's trailing whitespace after }; on line 76. Update the comment text and remove the trailing space to keep generated docs and formatting clean.
| // @param scenario: Scenario for which the backup to happen | |
| virtual Core::hresult Restore(const Scenario scenario) const = 0; | |
| }; | |
| // @param scenario: Scenario for which the restore to happen | |
| virtual Core::hresult Restore(const Scenario scenario) const = 0; | |
| }; |
docs/apis/Backup.md
Outdated
| | Name | Type | Description | | ||
| | :-------- | :-------- | :-------- | | ||
| | params | object | | | ||
| | params.scenario | string | Scenario for which the backup to happen | |
There was a problem hiding this comment.
Both restore and restoreSettings parameter descriptions incorrectly say 'backup'. These should describe the restore operation (e.g., 'Scenario for which the restore is to happen') to avoid confusion and mismatched generated docs.
docs/apis/Backup.md
Outdated
| | Name | Type | Description | | ||
| | :-------- | :-------- | :-------- | | ||
| | params | object | | | ||
| | params.scenario | string | Scenario for which the backup to happen | |
There was a problem hiding this comment.
Both restore and restoreSettings parameter descriptions incorrectly say 'backup'. These should describe the restore operation (e.g., 'Scenario for which the restore is to happen') to avoid confusion and mismatched generated docs.
| | params.scenario | string | Scenario for which the backup to happen | | |
| | params.scenario | string | Scenario for which the restore is to happen | |
docs/apis/Account.md
Outdated
| | Name | Type | Description | | ||
| | :-------- | :-------- | :-------- | | ||
| | result | object | | | ||
| | result.resetTime | integer | Time in UTC. Returns 0, if time is not available. | |
There was a problem hiding this comment.
IAccount uses uint64_t for resetTime, but the generated docs describe it only as integer (often interpreted as 32-bit). Update the documentation/type mapping to explicitly indicate a 64-bit integer (or document the unit/range) so API consumers don't truncate large epoch values.
| | result.resetTime | integer | Time in UTC. Returns 0, if time is not available. | | |
| | result.resetTime | integer (64-bit) | 64-bit unsigned integer representing epoch time in UTC (may exceed 32-bit range). Returns 0, if time is not available. | |
docs/apis/Account.md
Outdated
| | Name | Type | Description | | ||
| | :-------- | :-------- | :-------- | | ||
| | params | object | | | ||
| | params.resetTime | integer | Time in UTC. Returns 0, if time is not available. | |
There was a problem hiding this comment.
IAccount uses uint64_t for resetTime, but the generated docs describe it only as integer (often interpreted as 32-bit). Update the documentation/type mapping to explicitly indicate a 64-bit integer (or document the unit/range) so API consumers don't truncate large epoch values.
| | params.resetTime | integer | Time in UTC. Returns 0, if time is not available. | | |
| | params.resetTime | integer (int64) | Time in UTC as a 64-bit Unix epoch value (seconds since 1970-01-01T00:00:00Z). Returns 0 if time is not available. | |
apis/Account/IAccount.h
Outdated
| // @text getLastCheckoutResetTime | ||
| // @brief Gets the last reset time for Hotel Checkout. | ||
| // @param resetTime: Time in UTC. Returns 0, if time is not available. | ||
| virtual Core::hresult GetLastCheckoutResetTime(uint64_t& resetTime /* @out */) = 0; |
There was a problem hiding this comment.
GetLastCheckoutResetTime appears to be a read-only accessor but is not marked const, unlike SetLastCheckoutResetTime. Consider making it const for const-correctness and to make the interface easier to implement with immutable state.
| virtual Core::hresult GetLastCheckoutResetTime(uint64_t& resetTime /* @out */) = 0; | |
| virtual Core::hresult GetLastCheckoutResetTime(uint64_t& resetTime /* @out */) const = 0; |
anand-ky
left a comment
There was a problem hiding this comment.
Test review for checking if "Require conversation resolution before merging" works
Documentation Auto-GeneratedDocumentation has been automatically generated for the changed plugins and committed to this PR branch. The generated documentation files have been added to the PR for review. |
Documentation Auto-GeneratedDocumentation has been automatically generated for the changed plugins and committed to this PR branch. The generated documentation files have been added to the PR for review. |
5d59ef7 to
f49503b
Compare
| virtual ~IBackupManager() = default; | ||
|
|
||
| enum Scenario : uint8_t { | ||
| HOSPITALITY_RESET /* @text HOSPITALITY_RESET */ |
There was a problem hiding this comment.
Would just call it HOSPITALITY and strip the RESET
|
|
||
| virtual ~IBackupManager() = default; | ||
|
|
||
| enum Scenario : uint8_t { |
There was a problem hiding this comment.
Is Context a better name ?
Would also add a GENERIC entry as the first entry in the enum. Allowing for usage that does not require a context.
| // @brief Backup settings across the system | ||
| // @param scenario: Scenario for which the backup to happen | ||
| // @retval Core::ERROR_NONE Successfully backed up the settings | ||
| virtual Core::hresult BackupSettings(const Scenario scenario) = 0; |
There was a problem hiding this comment.
We should add a string to allow naming the backup. Enabling several backups of the same type.
E.g. A backup for english or french speeking hotel guests
|
|
||
| virtual ~IBackupProvider() = default; | ||
|
|
||
| enum Scenario : uint8_t { |
There was a problem hiding this comment.
not needed here, as we maintain in BackupManager
| // @brief Backup settings that belong to this component. | ||
| // @param scenario: Scenario for which the backup to happen | ||
| // @retval Core::ERROR_NONE Successfully backed up | ||
| virtual Core::hresult Backup(const Scenario scenario) = 0; |
There was a problem hiding this comment.
Add a name string as suggested on the manager.
on both Backup and Restore
| // @brief Backup settings across the system | ||
| // @param scenario: Scenario for which the backup to happen | ||
| // @retval Core::ERROR_NONE Successfully backed up the settings | ||
| virtual Core::hresult BackupSettings(const Scenario scenario) = 0; |
There was a problem hiding this comment.
Would Backup and Restore be better names also on the Manager
| // @param scenario: Scenario for which the backup to happen | ||
| // @retval Core::ERROR_NONE Successfully backed up the settings | ||
| virtual Core::hresult BackupSettings(const Scenario scenario) = 0; | ||
|
|
There was a problem hiding this comment.
With the intro of a name on the backups.
It would be good to have an iterator to list available backups.
| enum { ID = ID_BACKUP_MANAGER }; | ||
|
|
||
| virtual ~IBackupManager() = default; | ||
|
|
There was a problem hiding this comment.
I would think we need a delete method. to remove a named backup.
Reason for change: Defining Interface for new Plugins.
Test Procedure: Compile the interface repo and make sure document is getting generated.
Risks: Low
version: Major
Signed-off-by: Ramasamy Thalavay Pillai Ramasamy_ThalavayPillai@comcast.com