add some new property-with-value types #39064
Conversation
|
Is it still planned for this to go into v6.13? |
github-actions
bot
added
the
Has Conflicts
|
👋 Hi, @rboston628, Conflicts have been detected against the base branch. Please rebase your branch against the base branch. |
|
As this is failing tests and further commit(s) are expected we won't re-run any tests on the new Linux nodes. |
|
This pull request has been automatically marked as stale because it has not had activity in 3 months. It will be closed in 7 days if no further activity occurs. |
github-actions
bot
added
the
Stale
|
This pull request has been closed automatically. If these changes are still relevant then please re-open this pull request with a comment and tag @mantidproject/gatekeepers to highlight it needs to have a reviewer assigned. |
rboston628
force-pushed
the
patch-new-properties-with-values
branch
from
4c80143 to
af07b39
Compare
github-actions
bot
removed
the
Has Conflicts
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughSummary by CodeRabbit
Walkthrough
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Framework/Kernel/inc/MantidKernel/PropertyHelper.h (2)
67-79: Trailing delimiter in map toString output
toString(std::map<...>)currently appends the delimiter after the last entry. OthertoStringoverloads avoid trailing delimiters. Suggest aligning for consistency:-template <typename K, typename V> -std::string toString(const std::map<K, V> &value, const std::string &delimiter = ",") { - std::stringstream result; - // std::size_t vsize = value.size(); - for (auto it = value.begin(); it != value.end(); it++) { - result << "(" << toString(it->first) << ":"; - result << toString(it->second) << ")"; - result << delimiter; - } - return result.str(); -} +template <typename K, typename V> +std::string toString(const std::map<K, V> &value, const std::string &delimiter = ",") { + std::stringstream result; + for (auto it = value.begin(); it != value.end();) { + result << "(" << toString(it->first) << ":" << toString(it->second) << ")"; + if (++it != value.end()) { + result << delimiter; + } + } + return result.str(); +}
346-349: Intentional: += not implemented for mapsThrowing
NotImplementedErrorforstd::mapaligns with the current semantics for unsupported types. Consider documenting intended future behavior (merge vs append) if map support is planned beyond stringification/JSON encoding.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
Framework/Kernel/inc/MantidKernel/PropertyHelper.h(4 hunks)Framework/Kernel/inc/MantidKernel/PropertyWithValueJSON.h(1 hunks)Framework/Kernel/src/PropertyWithValue.cpp(1 hunks)Framework/Kernel/test/PropertyWithValueTest.h(1 hunks)Framework/PythonInterface/mantid/kernel/src/Exports/PropertyWithValue.cpp(1 hunks)
🔇 Additional comments (4)
Framework/Kernel/test/PropertyWithValueTest.h (1)
210-211: Comment reflow is fine; no behavioral impactThis is a pure comment reformat with no effect on test behavior. Safe change.
Framework/Kernel/src/PropertyWithValue.cpp (1)
97-98: Explicit instantiation added — consider ensuring headers cover std::mapThe instantiation looks correct and aligns with the new Python export. To avoid relying on transitive includes, ensure is included in a translation unit that sees this instantiation (here or via included headers) to prevent potential compile errors on stricter toolchains.
If needed, add near the includes:
#include <map>Framework/PythonInterface/mantid/kernel/src/Exports/PropertyWithValue.cpp (1)
43-49: Python export added — add include to avoid transitive include dependencyThe new export is consistent with existing patterns. To avoid relying on transitive includes, please include in this file (or ensure the exporter header includes it), since
std::map<int, std::vector<int>>is referenced.#include <map>Framework/Kernel/inc/MantidKernel/PropertyHelper.h (1)
236-239: Intentional: disallow string-to-map conversionThrowing
boost::bad_lexical_castforstd::mapparsing is a clear contract and prevents ambiguous string parsing. Looks good.
| template <typename K, typename V> | ||
| std::string toPrettyString(const std::map<K, V> &value, size_t maxLength = 0, bool collapseLists = true, | ||
| const std::string &delimiter = "\n", const std::string & = "+") { | ||
| std::stringstream retVal; | ||
| retVal << "{"; | ||
| for (auto it = value.begin(); it != value.end(); it++) { | ||
| retVal << toString(it->first) << ":" << toPrettyString(it->second, maxLength, collapseLists) << delimiter << " "; | ||
| } | ||
| return Strings::shorten(retVal.str(), maxLength); | ||
| } | ||
|
|
There was a problem hiding this comment.
Bug: toPrettyString(std::map<...>) misses closing brace and adds trailing delimiter
The output starts with “{” but never closes it, and it always appends the delimiter after the last item. Fix both and avoid trailing space:
-template <typename K, typename V>
-std::string toPrettyString(const std::map<K, V> &value, size_t maxLength = 0, bool collapseLists = true,
- const std::string &delimiter = "\n", const std::string & = "+") {
- std::stringstream retVal;
- retVal << "{";
- for (auto it = value.begin(); it != value.end(); it++) {
- retVal << toString(it->first) << ":" << toPrettyString(it->second, maxLength, collapseLists) << delimiter << " ";
- }
- return Strings::shorten(retVal.str(), maxLength);
-}
+template <typename K, typename V>
+std::string toPrettyString(const std::map<K, V> &value, size_t maxLength = 0, bool collapseLists = true,
+ const std::string &delimiter = "\n", const std::string & = "+") {
+ std::stringstream retVal;
+ retVal << "{";
+ for (auto it = value.begin(); it != value.end();) {
+ retVal << toString(it->first) << ":" << toPrettyString(it->second, maxLength, collapseLists);
+ if (++it != value.end()) {
+ retVal << delimiter << " ";
+ }
+ }
+ retVal << "}";
+ return Strings::shorten(retVal.str(), maxLength);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| template <typename K, typename V> | |
| std::string toPrettyString(const std::map<K, V> &value, size_t maxLength = 0, bool collapseLists = true, | |
| const std::string &delimiter = "\n", const std::string & = "+") { | |
| std::stringstream retVal; | |
| retVal << "{"; | |
| for (auto it = value.begin(); it != value.end(); it++) { | |
| retVal << toString(it->first) << ":" << toPrettyString(it->second, maxLength, collapseLists) << delimiter << " "; | |
| } | |
| return Strings::shorten(retVal.str(), maxLength); | |
| } | |
| template <typename K, typename V> | |
| std::string toPrettyString(const std::map<K, V> &value, size_t maxLength = 0, bool collapseLists = true, | |
| const std::string &delimiter = "\n", const std::string & = "+") { | |
| std::stringstream retVal; | |
| retVal << "{"; | |
| for (auto it = value.begin(); it != value.end();) { | |
| retVal << toString(it->first) << ":" | |
| << toPrettyString(it->second, maxLength, collapseLists); | |
| if (++it != value.end()) { | |
| retVal << delimiter << " "; | |
| } | |
| } | |
| retVal << "}"; | |
| return Strings::shorten(retVal.str(), maxLength); | |
| } |
🤖 Prompt for AI Agents
In Framework/Kernel/inc/MantidKernel/PropertyHelper.h around lines 131-141, the
map overload of toPrettyString opens a "{" but never appends the closing "}" and
always writes the delimiter (and a trailing space) after the last element;
update the implementation to iterate without adding the delimiter after the
final entry (for example detect last element with std::prev(value.end()) or
build the first-item flag and prepend delimiters for subsequent items), avoid
adding the extra trailing space, then append the closing "}" before calling
Strings::shorten; ensure the function returns the shortened full string
including the trailing brace.
| /** | ||
| * Encode a std::map value as a Json::Value dictionaryValue type | ||
| * @param mapValue The C++ value to encode | ||
| * @return A new Json::Value | ||
| */ | ||
| template <typename KeyType, typename ValueType> Json::Value encodeAsJson(const std::map<KeyType, ValueType> &mapValue) { | ||
| Json::Value jsonDictionary(Json::objectValue); | ||
| for (const auto &element : mapValue) { | ||
| jsonDictionary[element.first] = encodeAsJson(element.second); | ||
| } | ||
| return jsonDictionary; | ||
| } | ||
|
|
There was a problem hiding this comment.
Bug: Using non-string map keys for Json::Value object indexing
jsonDictionary[element.first] will select the array-indexing overload when KeyType is an integer (e.g., std::map<int, ...>). This is invalid for Json::objectValue and will assert/fail. JSON object keys must be strings.
Convert keys to strings before indexing. Example fix:
-template <typename KeyType, typename ValueType> Json::Value encodeAsJson(const std::map<KeyType, ValueType> &mapValue) {
- Json::Value jsonDictionary(Json::objectValue);
- for (const auto &element : mapValue) {
- jsonDictionary[element.first] = encodeAsJson(element.second);
- }
- return jsonDictionary;
-}
+template <typename KeyType, typename ValueType>
+Json::Value encodeAsJson(const std::map<KeyType, ValueType> &mapValue) {
+ Json::Value jsonDictionary(Json::objectValue);
+ for (const auto &element : mapValue) {
+ std::ostringstream keyStream;
+ keyStream << element.first; // supports numeric and streamable key types
+ jsonDictionary[keyStream.str()] = encodeAsJson(element.second);
+ }
+ return jsonDictionary;
+}And ensure the following headers are available in this TU:
#include <map>
#include <sstream>🤖 Prompt for AI Agents
In Framework/Kernel/inc/MantidKernel/PropertyWithValueJSON.h around lines
152-164, the template uses jsonDictionary[element.first] which will pick the
array-index overload when KeyType is non-string (e.g. int) and is invalid for
Json::objectValue; convert the map key to a std::string before indexing (e.g.
use std::ostringstream or std::to_string for arithmetic types) and assign the
encoded value to jsonDictionary[stringKey]; also ensure the translation unit
includes <map> and <sstream> (or <string>) so key-to-string conversion compiles.
|
👋 Hi, @rboston628, Conflicts have been detected against the base branch. Please rebase your branch against the base branch. |
github-actions
bot
added
the
Has Conflicts
Description of work
Summary of work
Fixes #xxxx.
Further detail of work
To test:
Reviewer
Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.
Code Review
Functional Tests
Does everything look good? Mark the review as Approve. A member of
@mantidproject/gatekeeperswill take care of it.Gatekeeper
If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.