Introduce a system for systematic definition of standard-defined attributes#8
Open
franzpoeschel wants to merge 90 commits intodevfrom
Open
Introduce a system for systematic definition of standard-defined attributes#8franzpoeschel wants to merge 90 commits intodevfrom
franzpoeschel wants to merge 90 commits intodevfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a scientific-defaults subsystem and a hierarchy visitor API; migrates per-type attribute init/read logic into configurable default readers/writers; adds traversal hooks across many model classes, new helpers/traits, implementation files, Python bindings, tests, and build entries to compile the new backend sources. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Series
participant Iteration
participant ScientificDefaults as SD (rgba(100,150,200,0.5))
participant ConfigAttribute as CA (rgba(120,180,140,0.5))
participant IOHandler as IO (rgba(200,120,140,0.5))
User->>Series: close()
Series->>Iteration: close()
Iteration->>SD: writeDefaults(standard)
SD->>Iteration: scientificDefaults_impl(Write, standard)
Iteration->>CA: defaultAttribute(name)
CA->>CA: write() if missing
CA->>Iteration: setAttribute(key, value)
Iteration->>IO: flush() / write attributes
User->>Series: open()/read()
Series->>Iteration: open()
Iteration->>IO: enqueue reads
IO->>Iteration: return raw attributes
Iteration->>SD: readDefaults(standard)
SD->>Iteration: scientificDefaults_impl(Read, standard)
Iteration->>CA: read() -> AttributeReader(s)
CA->>Iteration: setAttribute(convertedValue) or warn/error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/Attribute.cpp (1)
55-72:⚠️ Potential issue | 🟠 MajorDon’t alias
std::stringconversion eligibility tochar.This widens the matrix far beyond the intended
char/std::stringbridge. With the current mapping, any type that is convertible tocharalso becomes “eligible” forstd::string/std::vector<std::string>, andstd::stringbecomes eligible anywherecharis. That can pick the wrong reader or instantiate unsupporteddetail::doConvert<..., std::string>paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/Attribute.cpp` around lines 55 - 72, The code currently aliases std::string/std::vector<std::string> conversion eligibility to char by setting res[datatypeIndex<std::string>()] = res[datatypeIndex<char>()] and res[datatypeIndex<std::vector<std::string>>()] = res[datatypeIndex<char>()]; remove those two alias lines and instead explicitly compute eligibility for string types using std::is_convertible_v (i.e. set the string entry based on std::is_convertible_v<std::string, TargetType> and the vector<string> entry based on std::is_convertible_v<std::vector<std::string>, TargetType> or by mirroring the vector handling using datatypeIndex<std::vector<std::string>>() ), ensuring the logic in eligible_conversions/TargetType/datatypeIndex remains consistent and does not broaden string conversions via char.
🧹 Nitpick comments (2)
include/openPMD/backend/BaseRecord.hpp (1)
391-405: MakeIsBaseRecordresilient to cv/ref-qualified types.The current specialization only matches exact
BaseRecord<T>.const BaseRecord<T>&(and similar) evaluates to false, which can break template dispatch in generic code paths.♻️ Proposed trait hardening
+ template <typename T> + struct IsBaseRecordImpl + { + constexpr static bool value = false; + }; + template <typename T> - struct IsBaseRecord + struct IsBaseRecordImpl<BaseRecord<T>> { - constexpr static bool value = false; + constexpr static bool value = true; }; template <typename T> - struct IsBaseRecord<BaseRecord<T>> + struct IsBaseRecord { - constexpr static bool value = true; + constexpr static bool value = + IsBaseRecordImpl< + std::remove_cv_t<std::remove_reference_t<T>>>::value; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/openPMD/backend/BaseRecord.hpp` around lines 391 - 405, The IsBaseRecord trait only matches exact BaseRecord<T> and fails for cv/ref-qualified types; modify the specialization to strip cv/ref from the tested type (e.g. use std::remove_cvref_t or nested remove_reference_t/remove_cv_t) before checking for BaseRecord<T> so const BaseRecord<T>&, volatile BaseRecord<T>, etc. evaluate true; update IsBaseRecord, its specialization, and the IsBaseRecord_v alias in the detail namespace to use the cv/ref-normalized type when performing the match.test/CoreTest.cpp (1)
785-785: Drop the stray debug print.This will spam successful test runs and makes CI output harder to scan for real failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/CoreTest.cpp` at line 785, Remove the stray debug print that calls auxiliary::write_vec_to_stream(std::cout, m.attributes()) << std::endl; in the test; locate the call in CoreTest.cpp (the line using auxiliary::write_vec_to_stream and m.attributes()) and delete it (or wrap it behind a test-logging or DEBUG-only guard) so successful test runs no longer print attribute vectors to stdout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/openPMD/auxiliary/TypeTraits.hpp`:
- Around line 229-251: The SFINAE helper infer_template_args uses a non-const
reference (Base<Args...>&) which fails for cv-qualified types; add a
const-reference overload (e.g. Base<Args...> const&) and update the
IsTemplateBaseOf detection to instantiate
detail::infer_template_args<Base>(std::declval<std::remove_reference_t<T>
const&>()) so const-qualified and reference-qualified T are accepted; also
ensure <type_traits> is included for std::remove_reference_t. This touches the
infer_template_args declaration(s) and the IsTemplateBaseOf specialization using
detail::infer_template_args.
In `@include/openPMD/backend/Attribute.hpp`:
- Around line 118-119: Add a deprecated compatibility wrapper named get_impl()
that forwards to the new template getOrError() to avoid breaking downstream
code; implement get_impl() as a public member template that simply calls and
returns getOrError<U>(), mark it deprecated (e.g., with [[deprecated]] or a
macro used in the project) and keep the same signature std::variant<U,
std::runtime_error> get_impl() const so callers are preserved while nudging
users to switch to getOrError().
In `@include/openPMD/backend/HierarchyVisitor.hpp`:
- Around line 23-37: The HierarchyVisitor interface is missing a visitor
overload for PatchRecord; add a pure virtual method virtual void
operator()(PatchRecord &) = 0 to the HierarchyVisitor declaration and then
update HierarchyVisitorFromLambda to provide the matching forwarding/override
for operator()(PatchRecord &) so custom visitors and the lambda adapter can
observe PatchRecord nodes (ensure you reference the same PatchRecord type/name
used elsewhere and implement the forward call consistent with the other
operator() overloads).
In `@include/openPMD/backend/scientific_defaults/ProcessParsedAttribute.hpp`:
- Around line 7-10: The header ProcessParsedAttribute.hpp uses std::shared_ptr
but doesn't include <memory>, relying on transitive includes; add `#include`
<memory> to the header's includes so std::shared_ptr is defined (this will fix
usages of std::shared_ptr in this file such as the shared_ptr occurrences in
ProcessParsedAttribute.hpp).
In `@include/openPMD/backend/scientific_defaults/ProcessParsedAttribute.tpp`:
- Around line 81-89: The error message in the lambda (the std::runtime_error
handler in ProcessParsedAttribute.tpp that returns
std::optional<error::ReadError>) incorrectly always says "Expected a scalar
type" for non-scalar targets (e.g. require_type<std::vector<double>>() or
require_type<std::string>()); change the message to reflect the actual expected
target type (or a neutral phrasing like "Unexpected type for target" plus the
err.what() details) so dtype mismatches are reported correctly; update the
string built in that lambda (the one returning error::ReadError) to include the
requested/expected type information (or a neutral phrase) instead of the
hardcoded "Expected a scalar type".
In `@src/backend/Attributable.cpp`:
- Around line 306-339: populateMissingMetadata currently calls writeDefaults but
doesn't persist those changes; update the traversal so that after
component.writeDefaults(standard) the component's new attributes are
staged/persisted and made visible to readers (e.g., call the component-level
method that commits metadata or mark it dirty and invoke
IOHandler()->flush/commit for that component). Modify the lambda inside
visitHierarchyFromLambda in populateMissingMetadata to invoke the appropriate
persist/flush API (referencing populateMissingMetadata, writeDefaults,
visitHierarchyFromLambda, and IOHandler()) so defaults are not only written
in-memory but also persisted for concurrent/early readers.
In `@src/backend/BaseRecord.cpp`:
- Around line 779-782: The comment inside BaseRecord.cpp near the
ScientificDefaults::finalize() check contains a typo: change "populized" to
"populated" in the comment that reads about the Record being populated; keep the
rest of the sentence intact and ensure it still references "Record" and the
early return behavior.
In `@src/backend/scientific_defaults/AttributeReader.cpp`:
- Around line 37-43: The code currently calls (**processAttribute)(record,
attrName, a) after only checking processAttribute.has_value(), which can
dereference a null shared_ptr; update the guard in AttributeReader.cpp so you
first verify the contained pointer is non-null (e.g., check
processAttribute.has_value() && *processAttribute) before invoking the callback,
and handle the null case safely (skip invocation or return an appropriate error)
so the call to the callable (used to produce maybe_error) never dereferences a
null pointer.
In `@src/backend/scientific_defaults/ConfigAttribute.cpp`:
- Around line 60-69: The catch block for catch (error::ReadError const &e) is
treating all read errors as "attribute missing" and initializing defaults;
change it to only do that when the ReadError actually indicates the attribute
was not found. Inside the catch, inspect the error's failure kind (e.g.,
e.reason(), e.code(), or an isNotFound()/is_missing() helper on
error::ReadError) and only call
this->initDefaultAttribute.operator*()(this->attributable) and print the "Will
initialize..." message when that reason equals the backend's "not found" value;
for any other ReadError rethrow the exception (or propagate/log e.what()) so
corrupted/unreadable data stays visible. Ensure you keep references to
this->attrName and this->attributable.myPath().openPMDPath() in the messages
unchanged.
In `@src/backend/scientific_defaults/ScientificDefaults_auxiliary.cpp`:
- Around line 47-49: The function createDefaultAxisLabels currently groups case
0 and 1 and returns {"x"}, producing one label for 0D objects; change the switch
so case 0 returns an empty container ({}), and case 1 returns {"x"}, ensuring
axisLabels.size() equals the dimensionality; update the switch branch for
createDefaultAxisLabels to separate case 0 and case 1 accordingly.
In `@src/binding/python/Attributable.cpp`:
- Around line 650-654: The binding incorrectly references
&Iteration::populateMissingMetadata while defining the pybind11 method on the
Attributable class; change the binder to use
&Attributable::populateMissingMetadata so the Attributable py::class_ gets the
correct method. Locate the .def("populate_missing_metadata",
&Iteration::populateMissingMetadata, py::arg("recursive")) entry (adjacent to
.def("my_path", &Attributable::myPath)) and replace the &Iteration::... symbol
with &Attributable::populateMissingMetadata to avoid accidental future overrides
binding to the wrong class.
In `@src/Iteration.cpp`:
- Around line 972-982: The readers for time/dt/timeUnitSI currently use
get_float_types(), which rejects integer-valued attributes and breaks backwards
compatibility; change the reader filters to accept all numeric types (use
get_numeric_types() or the equivalent numeric-type helper) so integers are
allowed and then keep the existing converters (require_scalar for "time" and
"dt", require_type<double>() for "timeUnitSI") and the same setters
(Iteration::setTime, Iteration::setDt, Iteration::setTimeUnitSI) so numeric
integers will be accepted and converted as before.
In `@src/Mesh.cpp`:
- Around line 270-285: The defaultAttribute reader/writer mismatch:
defaultAttribute("gridUnitSI") always requires std::vector<double> but the else
branch uses the scalar setter Mesh::setGridUnitSI, so defaults written as a
scalar will fail to read; fix by changing the else branch reader to accept a
scalar double (use require_type<double>() for the withReader in the else branch)
and ensure the reader path invokes Mesh::setGridUnitSI (or converts the scalar
into the per-dimension vector before calling setGridUnitSIPerDimension) so
scalar defaults remain readable while the >=2.0 branch continues to require
std::vector<double>.
In `@src/Record.cpp`:
- Around line 227-229: The diff shows a duplicate registration of the
"timeOffset" attribute using defaultAttribute(..., "timeOffset") chained with
withSetter<Record>(0.f, &Record::setTimeOffset) and withReader(float_types,
require_scalar); remove this repeated registration so only the original
registration (the earlier defaultAttribute call that sets timeOffset) remains;
locate the duplicate call that uses defaultAttribute, withSetter<Record>,
Record::setTimeOffset and withReader and delete it to avoid reprocessing the
same attribute.
---
Outside diff comments:
In `@src/backend/Attribute.cpp`:
- Around line 55-72: The code currently aliases
std::string/std::vector<std::string> conversion eligibility to char by setting
res[datatypeIndex<std::string>()] = res[datatypeIndex<char>()] and
res[datatypeIndex<std::vector<std::string>>()] = res[datatypeIndex<char>()];
remove those two alias lines and instead explicitly compute eligibility for
string types using std::is_convertible_v (i.e. set the string entry based on
std::is_convertible_v<std::string, TargetType> and the vector<string> entry
based on std::is_convertible_v<std::vector<std::string>, TargetType> or by
mirroring the vector handling using datatypeIndex<std::vector<std::string>>() ),
ensuring the logic in eligible_conversions/TargetType/datatypeIndex remains
consistent and does not broaden string conversions via char.
---
Nitpick comments:
In `@include/openPMD/backend/BaseRecord.hpp`:
- Around line 391-405: The IsBaseRecord trait only matches exact BaseRecord<T>
and fails for cv/ref-qualified types; modify the specialization to strip cv/ref
from the tested type (e.g. use std::remove_cvref_t or nested
remove_reference_t/remove_cv_t) before checking for BaseRecord<T> so const
BaseRecord<T>&, volatile BaseRecord<T>, etc. evaluate true; update IsBaseRecord,
its specialization, and the IsBaseRecord_v alias in the detail namespace to use
the cv/ref-normalized type when performing the match.
In `@test/CoreTest.cpp`:
- Line 785: Remove the stray debug print that calls
auxiliary::write_vec_to_stream(std::cout, m.attributes()) << std::endl; in the
test; locate the call in CoreTest.cpp (the line using
auxiliary::write_vec_to_stream and m.attributes()) and delete it (or wrap it
behind a test-logging or DEBUG-only guard) so successful test runs no longer
print attribute vectors to stdout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa1d190d-a40e-4da6-aea6-d33b1673abc3
📒 Files selected for processing (56)
CMakeLists.txtinclude/openPMD/Iteration.hppinclude/openPMD/Mesh.hppinclude/openPMD/ParticlePatches.hppinclude/openPMD/ParticleSpecies.hppinclude/openPMD/Record.hppinclude/openPMD/RecordComponent.hppinclude/openPMD/Series.hppinclude/openPMD/auxiliary/TypeTraits.hppinclude/openPMD/backend/Attributable.hppinclude/openPMD/backend/Attribute.hppinclude/openPMD/backend/BaseRecord.hppinclude/openPMD/backend/BaseRecordComponent.hppinclude/openPMD/backend/Container.hppinclude/openPMD/backend/HierarchyVisitor.hppinclude/openPMD/backend/HierarchyVisitorImpl.hppinclude/openPMD/backend/MeshRecordComponent.hppinclude/openPMD/backend/PatchRecord.hppinclude/openPMD/backend/PatchRecordComponent.hppinclude/openPMD/backend/Writable.hppinclude/openPMD/backend/scientific_defaults/AttributeReader.hppinclude/openPMD/backend/scientific_defaults/ConfigAttribute.hppinclude/openPMD/backend/scientific_defaults/ConfigAttribute.tppinclude/openPMD/backend/scientific_defaults/ProcessParsedAttribute.hppinclude/openPMD/backend/scientific_defaults/ProcessParsedAttribute.tppinclude/openPMD/backend/scientific_defaults/ScientificDefaults.hppinclude/openPMD/backend/scientific_defaults/ScientificDefaults_auxiliary.hppinclude/openPMD/openPMD.hppinclude/openPMD/snapshots/ContainerImpls.hppsrc/IO/JSON/JSONIOHandlerImpl.cppsrc/Iteration.cppsrc/Mesh.cppsrc/ParticlePatches.cppsrc/ParticleSpecies.cppsrc/Record.cppsrc/RecordComponent.cppsrc/Series.cppsrc/backend/Attributable.cppsrc/backend/Attribute.cppsrc/backend/BaseRecord.cppsrc/backend/Container.cppsrc/backend/MeshRecordComponent.cppsrc/backend/PatchRecord.cppsrc/backend/PatchRecordComponent.cppsrc/backend/scientific_defaults/AttributeReader.cppsrc/backend/scientific_defaults/ConfigAttribute.cppsrc/backend/scientific_defaults/ProcessParsedAttribute.cppsrc/backend/scientific_defaults/ScientificDefaults.cppsrc/backend/scientific_defaults/ScientificDefaults_auxiliary.cppsrc/binding/python/Attributable.cppsrc/binding/python/Iteration.cppsrc/binding/python/PatchRecord.cppsrc/binding/python/Series.cppsrc/snapshots/ContainerImpls.cpptest/CoreTest.cpptest/python/unittest/API/APITest.py
Comment on lines
118
to
+119
| template <typename U> | ||
| std::variant<U, std::runtime_error> get_impl() const; | ||
| std::variant<U, std::runtime_error> getOrError() const; |
There was a problem hiding this comment.
Keep a deprecated alias for the renamed helper.
This is a public header, so replacing get_impl() with getOrError() is a compile-time break for downstream code. A deprecated forwarding wrapper would preserve source compatibility while still nudging callers to the new API.
Suggested compatibility shim
template <typename U>
std::variant<U, std::runtime_error> getOrError() const;
+
+ template <typename U>
+ [[deprecated("use getOrError()")]]
+ std::variant<U, std::runtime_error> get_impl() const
+ {
+ return getOrError<U>();
+ }📝 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.
Suggested change
| template <typename U> | |
| std::variant<U, std::runtime_error> get_impl() const; | |
| std::variant<U, std::runtime_error> getOrError() const; | |
| template <typename U> | |
| std::variant<U, std::runtime_error> getOrError() const; | |
| template <typename U> | |
| [[deprecated("use getOrError()")]] | |
| std::variant<U, std::runtime_error> get_impl() const | |
| { | |
| return getOrError<U>(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/openPMD/backend/Attribute.hpp` around lines 118 - 119, Add a
deprecated compatibility wrapper named get_impl() that forwards to the new
template getOrError() to avoid breaking downstream code; implement get_impl() as
a public member template that simply calls and returns getOrError<U>(), mark it
deprecated (e.g., with [[deprecated]] or a macro used in the project) and keep
the same signature std::variant<U, std::runtime_error> get_impl() const so
callers are preserved while nudging users to switch to getOrError().
Comment on lines
+47
to
+49
| case 0: | ||
| case 1: | ||
| return {"x"}; |
There was a problem hiding this comment.
Return no axis labels for 0D objects.
createDefaultAxisLabels(0) currently produces one label, which breaks the expected axisLabels.size() == dimensionality invariant and can emit invalid default metadata.
Suggested fix
- case 0:
- case 1:
+ case 0:
+ return {};
+ case 1:
return {"x"};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/scientific_defaults/ScientificDefaults_auxiliary.cpp` around
lines 47 - 49, The function createDefaultAxisLabels currently groups case 0 and
1 and returns {"x"}, producing one label for 0D objects; change the switch so
case 0 returns an empty container ({}), and case 1 returns {"x"}, ensuring
axisLabels.size() equals the dimensionality; update the switch branch for
createDefaultAxisLabels to separate case 0 and case 1 accordingly.
Bumps [conda-incubator/setup-miniconda](https://github.com/conda-incubator/setup-miniconda) from 3 to 4. - [Release notes](https://github.com/conda-incubator/setup-miniconda/releases) - [Changelog](https://github.com/conda-incubator/setup-miniconda/blob/main/CHANGELOG.md) - [Commits](conda-incubator/setup-miniconda@v3...v4) --- updated-dependencies: - dependency-name: conda-incubator/setup-miniconda dependency-version: '4' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| { | ||
| template <typename T, typename RecordType> | ||
| PostProcessConvertedAttributeImpl<T, RecordType>:: | ||
| PostProcessConvertedAttributeImpl(RecordType record_in, handler_t reader_in) |
Comment on lines
+354
to
+357
| // for (size_t i = 0; i < udims.size(); ++i) | ||
| // { | ||
| // res[7 * i] = 1; | ||
| // } |
Comment on lines
+699
to
+702
| // TODO: unitSI | ||
| // REQUIRE(r["x"].numAttributes() == 0); | ||
| // REQUIRE(r["y"].numAttributes() == 0); | ||
| // REQUIRE(r["z"].numAttributes() == 0); |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/openPMD/backend/scientific_defaults/ConfigAttribute.hpp`:
- Around line 7-9: Add direct STL includes for the symbols used: include
<functional> to cover std::function and include <type_traits> to cover
std::conditional_t and std::is_void_v; update the header that declares
ConfigAttribute (the file containing usages of std::function and
std::conditional_t / std::is_void_v) so it explicitly `#includes` these headers
rather than relying on transitive includes.
In `@include/openPMD/backend/scientific_defaults/ProcessParsedAttribute.hpp`:
- Around line 111-115: The declaration of require_type is missing the actual
'record' parameter present in the .tpp implementation; update the declaration of
template<typename T, typename RecordType> auto require_type(...) ->
std::shared_ptr<ProcessParsedAttribute> to include the record parameter with the
same type used in the implementation (e.g. add a parameter of type
std::remove_reference_t<RecordType>& record after the function-pointer argument)
so the declaration signature matches the require_type definition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24e15c64-0a0e-4539-b52f-f61d5636a250
📒 Files selected for processing (17)
include/openPMD/auxiliary/TypeTraits.hppinclude/openPMD/backend/Attributable.hppinclude/openPMD/backend/Attribute.hppinclude/openPMD/backend/HierarchyVisitor.hppinclude/openPMD/backend/HierarchyVisitorImpl.hppinclude/openPMD/backend/scientific_defaults/ConfigAttribute.hppinclude/openPMD/backend/scientific_defaults/ProcessParsedAttribute.hppinclude/openPMD/backend/scientific_defaults/ProcessParsedAttribute.tppsrc/Iteration.cppsrc/Mesh.cppsrc/Record.cppsrc/RecordComponent.cppsrc/backend/BaseRecord.cppsrc/backend/MeshRecordComponent.cppsrc/backend/scientific_defaults/AttributeReader.cppsrc/backend/scientific_defaults/ConfigAttribute.cppsrc/binding/python/Attributable.cpp
✅ Files skipped from review due to trivial changes (1)
- src/RecordComponent.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- src/backend/scientific_defaults/AttributeReader.cpp
- include/openPMD/auxiliary/TypeTraits.hpp
- src/backend/scientific_defaults/ConfigAttribute.cpp
- include/openPMD/backend/HierarchyVisitor.hpp
- src/Mesh.cpp
Comment on lines
+7
to
+9
| #include <deque> | ||
| #include <optional> | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n include/openPMD/backend/scientific_defaults/ConfigAttribute.hpp | head -60Repository: franzpoeschel/openPMD-api
Length of output: 2765
🏁 Script executed:
cat -n include/openPMD/backend/Attributable.hpp | head -20Repository: franzpoeschel/openPMD-api
Length of output: 1107
🏁 Script executed:
cat -n include/openPMD/backend/scientific_defaults/AttributeReader.hpp | head -20Repository: franzpoeschel/openPMD-api
Length of output: 638
🏁 Script executed:
cat -n include/openPMD/backend/scientific_defaults/ScientificDefaults_auxiliary.hpp | head -30Repository: franzpoeschel/openPMD-api
Length of output: 957
🏁 Script executed:
rg '#include.*functional' include/openPMD/backend/scientific_defaults/ConfigAttribute.hpp include/openPMD/backend/Attributable.hpp include/openPMD/backend/scientific_defaults/AttributeReader.hpp include/openPMD/backend/scientific_defaults/ScientificDefaults_auxiliary.hppRepository: franzpoeschel/openPMD-api
Length of output: 51
🏁 Script executed:
# Check what Attributable.hpp includes after the copyright header
sed -n '20,80p' include/openPMD/backend/Attributable.hppRepository: franzpoeschel/openPMD-api
Length of output: 1677
🏁 Script executed:
# Check if <functional> is included anywhere in the transitive includes
rg '#include.*functional' include/openPMD/backend/Attributable.hppRepository: franzpoeschel/openPMD-api
Length of output: 51
🏁 Script executed:
rg '#include.*functional' include/openPMD/backend/scientific_defaults/Repository: franzpoeschel/openPMD-api
Length of output: 51
🏁 Script executed:
# Let's trace the includes more systematically
# Check what ScientificDefaults_auxiliary.hpp includes
rg '^#include' include/openPMD/backend/scientific_defaults/ScientificDefaults_auxiliary.hppRepository: franzpoeschel/openPMD-api
Length of output: 189
🏁 Script executed:
# Check what AttributeReader.hpp includes
rg '^#include' include/openPMD/backend/scientific_defaults/AttributeReader.hppRepository: franzpoeschel/openPMD-api
Length of output: 313
🏁 Script executed:
# Check if any of the transitive includes pull in functional
rg '#include.*functional' include/openPMD/ -rRepository: franzpoeschel/openPMD-api
Length of output: 483
🏁 Script executed:
# Fix the rg command - search for functional in all headers
rg '#include.*functional' include/openPMD/ --type hRepository: franzpoeschel/openPMD-api
Length of output: 359
🏁 Script executed:
# Let's verify if type_traits is reliably available through transitive includes
# Check ProcessParsedAttribute.hpp
rg '^#include' include/openPMD/backend/scientific_defaults/ProcessParsedAttribute.hppRepository: franzpoeschel/openPMD-api
Length of output: 336
🏁 Script executed:
# Let's check the complete include chain for Attributable.hpp
cat -n include/openPMD/backend/Attributable.hpp | sed -n '1,40p'Repository: franzpoeschel/openPMD-api
Length of output: 1764
🏁 Script executed:
# Verify std::function and std::conditional_t are actually used in ConfigAttribute.hpp
grep -n "std::function\|std::conditional_t\|std::is_void_v" include/openPMD/backend/scientific_defaults/ConfigAttribute.hppRepository: franzpoeschel/openPMD-api
Length of output: 222
Add direct STL includes for used symbols.
std::function (Line 21) and std::conditional_t / std::is_void_v (Lines 45-46) are used without including <functional> and <type_traits>. While <type_traits> is available through transitive includes, <functional> is not, and relying on transitive dependencies is brittle.
Proposed fix
`#include` <deque>
+#include <functional>
`#include` <optional>
+#include <type_traits>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/openPMD/backend/scientific_defaults/ConfigAttribute.hpp` around lines
7 - 9, Add direct STL includes for the symbols used: include <functional> to
cover std::function and include <type_traits> to cover std::conditional_t and
std::is_void_v; update the header that declares ConfigAttribute (the file
containing usages of std::function and std::conditional_t / std::is_void_v) so
it explicitly `#includes` these headers rather than relying on transitive
includes.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Mesh.cpp (1)
354-375:⚠️ Potential issue | 🟠 MajorPreserve the spatial default when partially setting
gridUnitDimension.When the attribute is absent, this now seeds all axes with zero exponents. That makes untouched dimensions dimensionless on the first partial update, but callers build this attribute incrementally with empty maps meaning “keep the current/default value” (see
test/SerialIOTest.cpp:948-956). The initial state still needs to be spatial (L=1) per axis.Suggested fix
if (!this->containsAttribute("gridUnitDimension")) { - std::vector<double> res(udims.size() * 7); - // for (size_t i = 0; i < udims.size(); ++i) - // { - // res[7 * i] = 1; - // } + std::vector<double> res(udims.size() * 7, 0.0); + for (size_t i = 0; i < udims.size(); ++i) + { + res[7 * i] = 1.0; + } return res; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Mesh.cpp` around lines 354 - 375, The default construction of rawGridUnitDimension when containsAttribute("gridUnitDimension") is false currently yields zero exponents, which breaks the intended spatial default; change the initializer for res in the rawGridUnitDimension lambda (used with udims, rawGridUnitDimension, resize, cursor) to set the length exponent to 1 for each axis (i.e., set res[7*i] = 1 for each udim index) before returning so that fromMapOfUnitDimension(&*cursor, udim) and subsequent processing preserve the spatial default per axis when the attribute is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/openPMD/backend/Attributable.hpp`:
- Around line 434-435: Add the missing template implementation include so the
member template visitHierarchyFromLambda is available to users: after the
Attributable class definition in Attributable.hpp, add `#include`
"openPMD/backend/HierarchyVisitorImpl.hpp" (so the definition for
visitHierarchyFromLambda and related HierarchyVisitor implementations are
visible wherever Attributable is included).
In `@src/Mesh.cpp`:
- Around line 204-220: retrieveDimensionality() currently returns 1 when
begin()==end(), which misreports scalar N-D meshes; update the function to, when
no axisLabels and begin()==end(), query the dataset rank (e.g. datasetRank() or
equivalent API present in the class) and return that rank if >1, falling back to
1 only if the dataset rank is unknown or equals 1; keep references to
axisLabels(), begin(), end(), retrieveDimensionality(), and datasetRank()/rank
accessor to locate and implement the change.
- Around line 274-282: The v2+ branch registers only int_types for the
"gridUnitSI" defaultAttribute, so vector<float>/vector<long double> values
aren't converted to std::vector<double>; update the defaultAttribute setup for
"gridUnitSI" (the same block that calls defaultAttribute(*this, "gridUnitSI")
and uses Mesh::setGridUnitSIPerDimension) to mirror the pattern used for
gridSpacing/position by adding a withReader for float_types (and keeping
int_types) that reads require_type<std::vector<float>> and normalizes/returns a
std::vector<double> (so gridUnitSIPerDimension/getter sees double values);
ensure the float reader is registered alongside the existing int reader.
In `@src/ParticleSpecies.cpp`:
- Around line 30-36: The current ParticleSpecies::visitHierarchy visits
particlePatches outside the recursive branch and calls the visitor on this node
before its children when recursive==true; change the logic so that
particlePatches.visitHierarchy(v, recursive) is only executed inside the
recursive branch and placed before the call that invokes v on this object (i.e.,
ensure visitHierarchyImpl<ParticleSpecies>(v, recursive) / the code path that
ends up calling v(*this) happens after particlePatches.visitHierarchy(v,
recursive)) so the traversal is postfix and populateMissingMetadata(false) does
not descend into children unexpectedly.
In `@src/Series.cpp`:
- Around line 3320-3325: The loop in Series::close iterates over iterations and
calls Iteration::close(false), but Iteration::close(false) throws if the
iteration is in StepStatus::DuringStep; change the loop to skip iterations whose
status() == StepStatus::DuringStep so they are left for the existing
series-level finalization (impl.flush / flushStep) to finish, i.e., check each
iteration's status() before calling close(false) and only close iterations not
in DuringStep (alternatively, you may call the proper end-step method on the
Iteration before closing, but prefer skipping to let flush/flushStep handle
active steps).
---
Outside diff comments:
In `@src/Mesh.cpp`:
- Around line 354-375: The default construction of rawGridUnitDimension when
containsAttribute("gridUnitDimension") is false currently yields zero exponents,
which breaks the intended spatial default; change the initializer for res in the
rawGridUnitDimension lambda (used with udims, rawGridUnitDimension, resize,
cursor) to set the length exponent to 1 for each axis (i.e., set res[7*i] = 1
for each udim index) before returning so that fromMapOfUnitDimension(&*cursor,
udim) and subsequent processing preserve the spatial default per axis when the
attribute is missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24eae316-b80d-4be5-ab5e-5a4659b132b4
📒 Files selected for processing (12)
include/openPMD/backend/Attributable.hppinclude/openPMD/backend/scientific_defaults/ScientificDefaults.hppsrc/Iteration.cppsrc/Mesh.cppsrc/ParticleSpecies.cppsrc/Record.cppsrc/RecordComponent.cppsrc/Series.cppsrc/backend/MeshRecordComponent.cppsrc/backend/PatchRecord.cppsrc/backend/PatchRecordComponent.cppsrc/backend/scientific_defaults/ScientificDefaults.cpp
✅ Files skipped from review due to trivial changes (1)
- src/Record.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/backend/scientific_defaults/ScientificDefaults.cpp
Comment on lines
+204
to
+220
| auto Mesh::retrieveDimensionality() const -> uint64_t | ||
| { | ||
| uint64_t retrieveMeshDimensionality(Mesh const &m) | ||
| if (containsAttribute("axisLabels")) | ||
| { | ||
| if (m.containsAttribute("axisLabels")) | ||
| { | ||
| return m.axisLabels().size(); | ||
| } | ||
| return axisLabels().size(); | ||
| } | ||
|
|
||
| // maybe we have record components and can ask them | ||
| if (auto it = m.begin(); it != m.end()) | ||
| { | ||
| return it->second.getDimensionality(); | ||
| } | ||
| /* | ||
| * Since some backends cannot distinguish between vector and | ||
| * scalar values, the most likely answer here is 1. | ||
| */ | ||
| return 1; | ||
| // maybe we have record components and can ask them | ||
| if (auto it = begin(); it != end()) | ||
| { | ||
| return it->second.getDimensionality(); | ||
| } | ||
| /* | ||
| * Since some backends cannot distinguish between vector and | ||
| * scalar values, the most likely answer here is 1. | ||
| */ | ||
| return 1; |
There was a problem hiding this comment.
Handle scalar meshes in retrieveDimensionality().
For scalar meshes, begin() == end() even when the dataset rank is greater than 1, so this helper falls through to return 1. That shrinks every default/read fallback that depends on it (axisLabels, gridSpacing, gridGlobalOffset, gridUnitSI, gridUnitDimension) to length 1 on scalar N-D meshes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Mesh.cpp` around lines 204 - 220, retrieveDimensionality() currently
returns 1 when begin()==end(), which misreports scalar N-D meshes; update the
function to, when no axisLabels and begin()==end(), query the dataset rank (e.g.
datasetRank() or equivalent API present in the class) and return that rank if
>1, falling back to 1 only if the dataset rank is unknown or equals 1; keep
references to axisLabels(), begin(), end(), retrieveDimensionality(), and
datasetRank()/rank accessor to locate and implement the change.
Comment on lines
+274
to
+282
| if (standard >= OpenpmdStandard::v_2_0_0) | ||
| { | ||
| defaultAttribute(*this, "gridUnitSI") | ||
| .template withSetter<Mesh, std::vector<double> const &>( | ||
| [&]() { | ||
| return auxiliary::createDefaultVector(dimensionality, 1.); | ||
| }, | ||
| &Mesh::setGridUnitSIPerDimension) | ||
| .withReader(int_types, require_type<std::vector<double>>())(wor); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare reader coverage for the vector scientific-default attributes.
rg -nC3 'defaultAttribute\(\*this, "(gridUnitSI|gridSpacing|position)"\)|withReader\((float_types|int_types)' src/Mesh.cpp src/backend/MeshRecordComponent.cppRepository: franzpoeschel/openPMD-api
Length of output: 3143
Add the missing float-vector reader for v2 gridUnitSI.
The >= 2.0 branch only registers int_types. A valid gridUnitSI stored as vector<float> or vector<long double> will bypass normalization, while gridUnitSIPerDimension() expects std::vector<double>. This should mirror the float+int reader pattern used by gridSpacing and position.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Mesh.cpp` around lines 274 - 282, The v2+ branch registers only int_types
for the "gridUnitSI" defaultAttribute, so vector<float>/vector<long double>
values aren't converted to std::vector<double>; update the defaultAttribute
setup for "gridUnitSI" (the same block that calls defaultAttribute(*this,
"gridUnitSI") and uses Mesh::setGridUnitSIPerDimension) to mirror the pattern
used for gridSpacing/position by adding a withReader for float_types (and
keeping int_types) that reads require_type<std::vector<float>> and
normalizes/returns a std::vector<double> (so gridUnitSIPerDimension/getter sees
double values); ensure the float reader is registered alongside the existing int
reader.
Comment on lines
+3320
to
+3325
| for (auto &[_, iteration] : iterations) | ||
| { | ||
| (void)_; | ||
| if (!iteration.closed()) | ||
| { | ||
| iteration.close(/* flush = */ false); |
There was a problem hiding this comment.
Don't route active-step iterations through close(false) here.
Iteration::close(false) now throws when the iteration is still in StepStatus::DuringStep (see src/Iteration.cpp, Line 164), so this pre-flush loop can make Series::close() / destruction fail before impl.flush() and flushStep() run. That turns an otherwise recoverable finalization path into a hard cleanup failure. Either skip DuringStep iterations here and let the existing series-level flush logic finish them, or explicitly end the step before marking the iteration closed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Series.cpp` around lines 3320 - 3325, The loop in Series::close iterates
over iterations and calls Iteration::close(false), but Iteration::close(false)
throws if the iteration is in StepStatus::DuringStep; change the loop to skip
iterations whose status() == StepStatus::DuringStep so they are left for the
existing series-level finalization (impl.flush / flushStep) to finish, i.e.,
check each iteration's status() before calling close(false) and only close
iterations not in DuringStep (alternatively, you may call the proper end-step
method on the Iteration before closing, but prefer skipping to let
flush/flushStep handle active steps).
Moved to finalize(), degraded to warning
prep for reading
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/backend/BaseRecord.cpp (1)
778-787:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep skeleton-only flushes dirty.
This branch now clears dirty unconditionally, but the normal path at Line 801 only clears it for non-
FlushLevel::SkeletonOnlyflushes. After a skeleton-only pass, a later full flush will short-circuit at Line 773 unless something else re-dirties the record, so this record can be skipped permanently.Suggested adjustment
if (!this->written() && this->empty() && !this->datasetDefined()) { // Verify upon ScientificDefaults::finalize() that the Record has been // populated. For now, we will assume that data will come later; ignore // this Record at the moment. // // If any of the properties above will change, the Record will become // dirty through it. - this->setDirty(false); + if (flushParams.flushLevel != FlushLevel::SkeletonOnly) + { + this->setDirty(false); + } return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/BaseRecord.cpp` around lines 778 - 787, The code branch that calls this->setDirty(false) when !written() && empty() && !datasetDefined() must preserve dirty state for Skeleton-only flushes; change the condition so setDirty(false) is only called when the current flush level is not FlushLevel::SkeletonOnly (e.g., check the flush level parameter or member used by ScientificDefaults::finalize()), leaving the record dirty after a SkeletonOnly pass; locate the check around written()/empty()/datasetDefined() and gate the setDirty(false) call on flushLevel != FlushLevel::SkeletonOnly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/RecordComponent.cpp`:
- Around line 257-261: The branch that assigns rc.m_dataset = std::move(d)
currently calls setDirty(true), causing a Dataset{Datatype::UNDEFINED,...} to be
treated as a configured dataset and later enqueued by flush(); to fix, avoid
marking the object dirty when the new dataset has Datatype::UNDEFINED (i.e.,
only call setDirty(true) if d.datatype != Datatype::UNDEFINED), or alternatively
keep setDirty but add a guard in flush() that skips enqueuing CREATE_DATASET
when rc.m_dataset.datatype == Datatype::UNDEFINED; update the code around
rc.m_dataset, setDirty, and flush()/CREATE_DATASET to implement one of these
approaches so an unset dataset is not flushed.
- Around line 707-711: The reader for the "unitSI" attribute currently uses
get_numerical_types(), which lets integer-typed metadata pass; change it to use
only floating-point attribute types (mirror the pattern used in
PatchRecordComponent) so that defaultAttribute(... "unitSI").withReader(...)
accepts only floating types. Locate the block around RecordComponent,
defaultAttribute, setUnitSI and replace get_numerical_types() with the
floating-only type set (e.g., get_floating_point_types() or the project’s
equivalent) so the withReader(...) call is restricted to floats (keeping
require_type<double>() as the concrete checker).
---
Duplicate comments:
In `@src/backend/BaseRecord.cpp`:
- Around line 778-787: The code branch that calls this->setDirty(false) when
!written() && empty() && !datasetDefined() must preserve dirty state for
Skeleton-only flushes; change the condition so setDirty(false) is only called
when the current flush level is not FlushLevel::SkeletonOnly (e.g., check the
flush level parameter or member used by ScientificDefaults::finalize()), leaving
the record dirty after a SkeletonOnly pass; locate the check around
written()/empty()/datasetDefined() and gate the setDirty(false) call on
flushLevel != FlushLevel::SkeletonOnly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 890f4c64-d6be-4381-8965-49ce3aaf5958
📒 Files selected for processing (3)
src/RecordComponent.cppsrc/backend/BaseRecord.cppsrc/backend/BaseRecordComponent.cpp
Comment on lines
257
to
261
| else | ||
| { | ||
| rc.m_dataset = std::move(d); | ||
| setDirty(true); | ||
| return *this; |
There was a problem hiding this comment.
Do not dirty the “unset dataset” state.
Lines 259-260 now mark Dataset{Datatype::UNDEFINED, ...} as dirty, but flush() still treats any present m_dataset as flushable. That makes this branch enqueue dataset creation for an undefined datatype instead of behaving like “not configured yet”. Please either keep this branch non-dirty or add a Datatype::UNDEFINED guard before CREATE_DATASET.
Suggested fix
else
{
rc.m_dataset = std::move(d);
- setDirty(true);
return *this;
}📝 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.
Suggested change
| else | |
| { | |
| rc.m_dataset = std::move(d); | |
| setDirty(true); | |
| return *this; | |
| else | |
| { | |
| rc.m_dataset = std::move(d); | |
| return *this; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/RecordComponent.cpp` around lines 257 - 261, The branch that assigns
rc.m_dataset = std::move(d) currently calls setDirty(true), causing a
Dataset{Datatype::UNDEFINED,...} to be treated as a configured dataset and later
enqueued by flush(); to fix, avoid marking the object dirty when the new dataset
has Datatype::UNDEFINED (i.e., only call setDirty(true) if d.datatype !=
Datatype::UNDEFINED), or alternatively keep setDirty but add a guard in flush()
that skips enqueuing CREATE_DATASET when rc.m_dataset.datatype ==
Datatype::UNDEFINED; update the code around rc.m_dataset, setDirty, and
flush()/CREATE_DATASET to implement one of these approaches so an unset dataset
is not flushed.
Comment on lines
+707
to
+711
| auto numerical_types = get_numerical_types(); | ||
|
|
||
| defaultAttribute(*this, "unitSI") | ||
| .template withSetter<RecordComponent>(1.0, &RecordComponent::setUnitSI) | ||
| .withReader(numerical_types, require_type<double>())(wor); |
There was a problem hiding this comment.
Keep unitSI reader restricted to floating-point attribute types.
Using get_numerical_types() here silently accepts integer-typed unitSI metadata. That weakens standards validation compared with the existing unitSI pattern in src/backend/PatchRecordComponent.cpp:83-98, which is explicitly based on float types.
Suggested fix
- auto numerical_types = get_numerical_types();
+ auto float_types = get_float_types();
defaultAttribute(*this, "unitSI")
.template withSetter<RecordComponent>(1.0, &RecordComponent::setUnitSI)
- .withReader(numerical_types, require_type<double>())(wor);
+ .withReader(float_types, require_type<double>())(wor);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/RecordComponent.cpp` around lines 707 - 711, The reader for the "unitSI"
attribute currently uses get_numerical_types(), which lets integer-typed
metadata pass; change it to use only floating-point attribute types (mirror the
pattern used in PatchRecordComponent) so that defaultAttribute(...
"unitSI").withReader(...) accepts only floating types. Locate the block around
RecordComponent, defaultAttribute, setUnitSI and replace get_numerical_types()
with the floating-only type set (e.g., get_floating_point_types() or the
project’s equivalent) so the withReader(...) call is restricted to floats
(keeping require_type<double>() as the concrete checker).
retrigger CI
available on all attributables
franzpoeschel
force-pushed
the
defaults-upon-close
branch
from
4490bbf to
8d5fb44
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes