Data Exchange, Step Export - Preserving control directives#601
Merged
dpasukhi merged 2 commits intoOpen-Cascade-SAS:IRfrom Jul 12, 2025
Merged
Data Exchange, Step Export - Preserving control directives#601dpasukhi merged 2 commits intoOpen-Cascade-SAS:IRfrom
dpasukhi merged 2 commits intoOpen-Cascade-SAS:IRfrom
Conversation
Fixed issue when control directives inside existed text is corrupted during export. Added GTest to validate conversion
There was a problem hiding this comment.
Pull Request Overview
This PR refactors text handling in STEP export to correctly escape special characters while preserving existing control directives, integrates the new logic into the Send method, and adds GTest coverage for the conversion.
- Introduced
CleanTextForSendstatic helper with detailed documentation. - Updated
StepData_StepWriter::Sendto use the new helper and simplified quoting/line‐wrapping logic. - Added comprehensive GTests for
CleanTextForSendand updated the test suite configuration.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/DataExchange/TKDESTEP/StepData/StepData_StepWriter.hxx | Declares CleanTextForSend helper with doc comments |
| src/DataExchange/TKDESTEP/StepData/StepData_StepWriter.cxx | Implements CleanTextForSend, refactors Send, removes old logic |
| src/DataExchange/TKDESTEP/GTests/StepData_StepWriter_Test.cxx | Adds GTests covering CleanTextForSend scenarios |
| src/DataExchange/TKDESTEP/GTests/FILES.cmake | Registers the new test file in CMake |
Comments suppressed due to low confidence (4)
src/DataExchange/TKDESTEP/StepData/StepData_StepWriter.hxx:278
- The doc comment shows syntax with braces (
\X{HH}\) but the implementation handles\XHH\without braces. Update the comment to match actual supported pattern or enforce brace syntax.
//! - \X{HH}\ : Single byte character encoding (U+0000 to U+00FF)
src/DataExchange/TKDESTEP/GTests/StepData_StepWriter_Test.cxx:19
- Current tests only cover
CleanTextForSend. You should add tests forStepData_StepWriter::Sendto verify surrounding quotes and line-wrapping behavior.
// Test CleanTextForSend with basic character escaping
src/DataExchange/TKDESTEP/StepData/StepData_StepWriter.cxx:833
- [nitpick] Variable name
aNnis not descriptive. Consider renaming tocleanedLengthor similar for readability.
Standard_Integer aNn = aVal.Length();
src/DataExchange/TKDESTEP/StepData/StepData_StepWriter.cxx:1212
- The code matches
\XHH\sequences but the header comment refers to\X{HH}\. Consider clarifying in code or docs whether braces are required or optional.
else if (anI + 4 <= aNb && theText.Value(anI + 1) == 'X' && theText.Value(anI + 4) == '\\')
Comment on lines
+1218
to
+1223
| Standard_Boolean aThirdIsHex = | ||
| ((aThirdChar >= '0' && aThirdChar <= '9') || (aThirdChar >= 'A' && aThirdChar <= 'F') | ||
| || (aThirdChar >= 'a' && aThirdChar <= 'f')); | ||
| Standard_Boolean aFourthIsHex = | ||
| ((aFourthChar >= '0' && aFourthChar <= '9') || (aFourthChar >= 'A' && aFourthChar <= 'F') | ||
| || (aFourthChar >= 'a' && aFourthChar <= 'f')); |
Collaborator
There was a problem hiding this comment.
I think these two checks can be replaced with std::isxdigit()
| } | ||
| } | ||
| // Check for \P{char}\ patterns (need exactly 4 characters: \P{char}\) | ||
| else if (anI + 3 <= aNb && theText.Value(anI + 1) == 'P' && theText.Value(anI + 3) == '\\') |
Collaborator
There was a problem hiding this comment.
Shouldn´t we check 2nd character for std::isalpha?
dpasukhi
added a commit
that referenced
this pull request
Sep 6, 2025
- Introduced `CleanTextForSend` static helper with detailed documentation. - Updated `StepData_StepWriter::Send` to use the new helper and simplified quoting/line‐wrapping logic. - Added comprehensive GTests for `CleanTextForSend` and updated the test suite configuration.
dpasukhi
added a commit
that referenced
this pull request
Sep 6, 2025
- Introduced `CleanTextForSend` static helper with detailed documentation. - Updated `StepData_StepWriter::Send` to use the new helper and simplified quoting/line‐wrapping logic. - Added comprehensive GTests for `CleanTextForSend` and updated the test suite configuration.
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
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.
Fixed issue when control directives inside existed text is corrupted during export.
Added GTest to validate conversion