Skip to content

config: enable configuration override for publisher history length#226

Merged
VDanielEdwards merged 3 commits into
mainfrom
dev/config/allow-publisher-history-configuration
Jul 11, 2025
Merged

config: enable configuration override for publisher history length#226
VDanielEdwards merged 3 commits into
mainfrom
dev/config/allow-publisher-history-configuration

Conversation

@VDanielEdwards
Copy link
Copy Markdown
Member

Subject

Makes it possible to override the publisher history length in the participant configuration. Analogous to the override of the topic, or the labels.

JIRA: SILKIT-1777

Developer checklist (address before review)

  • Changelog.md updated
  • Prepared update for depending repositories
  • Documentation updated (public API changes only)
  • API docstrings updated (public API changes only)
  • Rebase → commit history clean
  • [x Squash and merge → proper PR title

@VDanielEdwards VDanielEdwards self-assigned this Jul 7, 2025
@VDanielEdwards VDanielEdwards force-pushed the dev/config/allow-publisher-history-configuration branch from 00c45f6 to 88a6821 Compare July 9, 2025 07:20
@MariusBgm MariusBgm requested review from MariusBgm and Copilot July 9, 2025 08:09
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Enables overriding the publisher history length in the participant configuration, mirroring existing overrides for topic and labels.

  • Adds an optional history field to DataPublisher in the participant config
  • Integrates the override into publisher creation (update, validation, and application)
  • Updates YAML reader/writer/validator/schema and adds integration tests

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
docs/CHANGELOG.rst Document the new history-length override feature
SilKit/source/core/participant/Participant_impl.hpp Apply the history override, validate and set history
SilKit/source/config/YamlWriter.cpp Serialize history field
SilKit/source/config/YamlValidator.cpp Include History in schema paths
SilKit/source/config/YamlReader.cpp Parse history field
SilKit/source/config/ParticipantConfiguration.hpp Add optional history member
SilKit/source/config/ParticipantConfiguration.schema.json Define and reference the History property
SilKit/source/config/ParticipantConfiguration_Full.yaml/json Add sample History entries
SilKit/IntegrationTests/ITest_PubHistory.cpp Add tests covering API and config override behavior
SilKit/IntegrationTests/CMakeLists.txt Include the new test in the integration suite
Comments suppressed due to low confidence (2)

SilKit/IntegrationTests/ITest_PubHistory.cpp:229

  • Consider adding a test for an invalid configuration (e.g. History: 2) to verify that the configuration error is properly thrown or caught.
}

SilKit/IntegrationTests/ITest_PubHistory.cpp:117

  • [nitpick] The test name uses 'conf' as shorthand for 'configuration'; consider renaming to 'config' for clarity (e.g. history_api_one_config_empty).
TEST_F(ITest_PubHistory, history_api_one_conf_empty)

Comment thread SilKit/source/core/participant/Participant_impl.hpp
Comment thread SilKit/source/core/participant/Participant_impl.hpp
@VDanielEdwards
Copy link
Copy Markdown
Member Author

VDanielEdwards commented Jul 9, 2025

But I just noticed I missed the configuration-includes processing. It's fine - controller 'blocks' are not merged, just the lists of controllers.

Copy link
Copy Markdown
Collaborator

@MariusBgm MariusBgm left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread SilKit/IntegrationTests/ITest_PubHistory.cpp
@VDanielEdwards VDanielEdwards merged commit af38b09 into main Jul 11, 2025
13 checks passed
@VDanielEdwards VDanielEdwards deleted the dev/config/allow-publisher-history-configuration branch July 11, 2025 08:04
SimplyLMK pushed a commit to SimplyLMK/sil-kit that referenced this pull request Oct 23, 2025
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