ЗаписатьТекущий пишет только текущее состояние #1583
Conversation
WalkthroughReplaced XmlWriterImpl.WriteCurrent single-node copy with node-type-driven handlers and helpers for elements/attributes/doctype; refactored XmlReader.Read into ReadInternal with special-casing for XmlDeclaration/DocumentType and optional skipping; made DtdProcessing explicit in settings; added extensive WriteCurrent tests and case-insensitive test assertions/diffing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Reader as XmlReaderImpl
participant Writer as XmlWriterImpl
Caller->>Writer: WriteCurrent(Reader)
Writer->>Reader: inspect NodeType.UnderlyingValue
alt Element
Note over Writer: CopyElementAndAttributes (start element + attributes)
Writer->>Writer: WriteStartElement(reader.Name)
loop attributes
Writer->>Reader: AttributeName(i) / GetAttribute(i)
Writer->>Writer: CopyAttribute(name, value,...)
end
else XmlDeclaration
Writer->>Writer: WriteXMLDeclaration()
else DocumentType
Writer->>Writer: CopyDocumentType(reader, writer)
else Attribute
Writer->>Writer: WriteAttribute(localName, prefix, value)
else EndElement
Writer->>Writer: WriteEndElement()
else CDATA/Text/Whitespace/Comment
Writer->>Writer: WriteCDATASection/WriteText/WriteComment(...)
else Unsupported
Writer->>Writer: throw RuntimeException("Копирование узла ... не поддерживается")
end
sequenceDiagram
autonumber
actor Caller
participant Reader as XmlReaderImpl
Caller->>Reader: Read()
Reader->>Reader: ReadInternal()
alt Node is XmlDeclaration or DocumentType
Reader->>Reader: advance once
loop skip whitespace
Reader->>Reader: advance if Whitespace/SignificantWhitespace
end
Note over Reader: return status (no CheckEmptyElementEntering)
else Other nodes
Reader->>Reader: normal ReadWhenStateChanged()/XmlReader.Read()
Reader->>Reader: CheckEmptyElementEntering()
end
Note right of Reader: If IgnoreXMLDeclaration true and XmlDeclaration found, Read() may call ReadInternal() again to skip it
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OneScript.StandardLibrary/Xml/XmlReaderImpl.cs (1)
399-424: ReadInternal unconditionally skips XmlDeclaration/DocumentType and whitespace, ignoring settings; also bypasses empty-element bookkeeping.
- Always skipping DocumentType disregards IgnoreDocumentType/DtdProcessing.
- Whitespace is skipped unconditionally after special nodes, even when IgnoreWhitespace=false.
- Empty-element state (CheckEmptyElementEntering) isn’t called after skipping, altering downstream behavior.
Refactor to respect settings, skip only when requested, and preserve empty-element handling.
- private bool ReadInternal() - { - if (_reader == null) - return false; - - if (_emptyElemReadState == EmptyElemCompabilityState.EmptyElementEntered) - { - _emptyElemReadState = EmptyElemCompabilityState.EmptyElementRead; - return true; - } - else if (_reader.NodeType == XmlNodeType.XmlDeclaration || _reader.NodeType == XmlNodeType.DocumentType) - { - var readingDone = _reader.Read(); - while (readingDone && _reader.NodeType == XmlNodeType.Whitespace) - { - readingDone = _reader.Read(); - } - return readingDone; - } - else - { - bool readingDone = _ignoreWSChanged ? ReadWhenStateChanged() : _reader.Read(); - CheckEmptyElementEntering(); - return readingDone; - } - } + private bool ReadInternal() + { + if (_reader == null) + return false; + + if (_emptyElemReadState == EmptyElemCompabilityState.EmptyElementEntered) + { + _emptyElemReadState = EmptyElemCompabilityState.EmptyElementRead; + return true; + } + + bool readingDone = _ignoreWSChanged ? ReadWhenStateChanged() : _reader.Read(); + if (!readingDone) + return false; + + // Optionally skip XML declaration + if (_reader.NodeType == XmlNodeType.XmlDeclaration && _settings.IgnoreXMLDeclaration) + { + do + { + readingDone = _ignoreWSChanged ? ReadWhenStateChanged() : _reader.Read(); + } + while (readingDone && (_reader.NodeType == XmlNodeType.XmlDeclaration + || (_ignoreWhitespace && _reader.NodeType == XmlNodeType.Whitespace))); + if (readingDone) CheckEmptyElementEntering(); + return readingDone; + } + + // Optionally skip DOCTYPE + if (_reader.NodeType == XmlNodeType.DocumentType && _settings.IgnoreDocumentType) + { + do + { + readingDone = _ignoreWSChanged ? ReadWhenStateChanged() : _reader.Read(); + } + while (readingDone && (_ignoreWhitespace && _reader.NodeType == XmlNodeType.Whitespace)); + if (readingDone) CheckEmptyElementEntering(); + return readingDone; + } + + CheckEmptyElementEntering(); + return readingDone; + }
♻️ Duplicate comments (1)
src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs (1)
186-210: Don't silently drop node types (CDATA, Comment, PI, Whitespace, EntityRef).Current switch ignores several legitimate XmlNodeType values, so such nodes won’t be written at all. This is a correctness regression vs. WriteNode and matches a prior concern in the thread. Add explicit handling to keep WriteCurrent faithful for those nodes.
switch (nodeType) { case XmlNodeType.Element: CopyElementAndAttributes(reader, this); break; case XmlNodeType.XmlDeclaration: WriteXMLDeclaration(); break; case XmlNodeType.DocumentType: CopyDocumentType(reader, this); break; case XmlNodeType.Attribute: WriteAttribute(reader.Name, reader.Value); break; case XmlNodeType.EndElement: WriteEndElement(); break; case XmlNodeType.Text: WriteText(reader.Value); break; + case XmlNodeType.CDATA: + WriteCDATASection(reader.Value); + break; + case XmlNodeType.Comment: + WriteComment(reader.Value); + break; + case XmlNodeType.ProcessingInstruction: + WriteProcessingInstruction(reader.Name, reader.Value); + break; + case XmlNodeType.Whitespace: + case XmlNodeType.SignificantWhitespace: + _writer.WriteWhitespace(reader.Value); + break; + case XmlNodeType.EntityReference: + WriteEntityReference(reader.Name); + break; default: break; }
🧹 Nitpick comments (5)
tests/testrunner.os (2)
85-95: Avoid type errors by validating inputs before calling нрег; drop unreachable fallback.Calling нрег(...) before confirming both args are strings can raise a type error. Validate types first and use the detailed diff error once; the generic message below is unreachable for string inputs.
Процедура ПроверитьРавенствоСтрокБезУчетаРегистра(ПервоеЗначение, ВтороеЗначение, ДопСообщениеОшибки = "") Экспорт - Если нрег(ПервоеЗначение) <> нрег(ВтороеЗначение) Тогда - Если ТипЗнч(ПервоеЗначение) = Тип("Строка") и ТипЗнч(ВтороеЗначение) = Тип("Строка") Тогда - ВызватьИсключение ИсключениеНеравенстваСтрок(ПервоеЗначение, ВтороеЗначение, Истина) + " - |" + ДопСообщениеОшибки; - КонецЕсли; - - СообщениеОшибки = "Сравниваемые значения ("+ПервоеЗначение+"; "+ВтороеЗначение+") не равны, а хотели, чтобы были равны." + ФорматДСО(ДопСообщениеОшибки); - ВызватьИсключение(СообщениеОшибки); - КонецЕсли; + Если ТипЗнч(ПервоеЗначение) <> Тип("Строка") ИЛИ ТипЗнч(ВтороеЗначение) <> Тип("Строка") Тогда + СообщениеОшибки = "Сравниваемые значения ("+ПервоеЗначение+"; "+ВтороеЗначение+") не равны, а хотели, чтобы были равны." + ФорматДСО(ДопСообщениеОшибки); + ВызватьИсключение(СообщениеОшибки); + КонецЕсли; + Если нрег(ПервоеЗначение) <> нрег(ВтороеЗначение) Тогда + ВызватьИсключение ИсключениеНеравенстваСтрок(ПервоеЗначение, ВтороеЗначение, Истина) + " + |" + ДопСообщениеОшибки; + КонецЕсли; КонецПроцедуры
211-259: Good diff output; micro-optimizations for case-insensitive path.Pre-normalize both strings once when БезУчетаРегистра=Истина to avoid per-char нрег calls; comparison logic stays the same.
Функция ИсключениеНеравенстваСтрок(ПервоеЗначение, ВтороеЗначение, Знач БезУчетаРегистра = Ложь) @@ - МинДлина = Мин(СтрДлина(ПервоеЗначение), СтрДлина(ВтороеЗначение)); + СтрСравн1 = ПервоеЗначение; + СтрСравн2 = ВтороеЗначение; + Если БезУчетаРегистра Тогда + СтрСравн1 = нрег(СтрСравн1); + СтрСравн2 = нрег(СтрСравн2); + КонецЕсли; + + МинДлина = Мин(СтрДлина(СтрСравн1), СтрДлина(СтрСравн2)); ИндексРазличия = МинДлина + 1; Для Сч = 1 По МинДлина Цикл - - Символ1 = Сред(ПервоеЗначение, Сч, 1); - Символ2 = Сред(ВтороеЗначение, Сч, 1); - - Если БезУчетаРегистра Тогда - Символ1 = нрег(Символ1); - Символ2 = нрег(Символ2); - КонецЕсли; + Символ1 = Сред(СтрСравн1, Сч, 1); + Символ2 = Сред(СтрСравн2, Сч, 1);src/OneScript.StandardLibrary/Xml/XmlReaderSettingsImpl.cs (1)
126-134: Harden defaults against XXE.Consider explicitly disabling external resolution to reduce XXE risk, even when DTD parsing is enabled elsewhere.
var settings = new XmlReaderSettings { ValidationType = ValidationType.None, IgnoreComments = true, // отличается от конструктора скрипта IgnoreProcessingInstructions = false, IgnoreWhitespace = true, DtdProcessing = DtdProcessing.Ignore, + XmlResolver = null, };Apply the same in the script constructor block if you ever allow Parse.
src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs (2)
212-216: DOCTYPE: only internal subset is preserved; consider PUBLIC/SYSTEM IDs.CopyDocumentType(reader.Name, reader.Value) maps Value to internal subset and drops public/system identifiers. If XmlReaderImpl exposes them, prefer passing all parts to WriteDocumentType.
If available, implement:
- writer.WriteDocumentType(reader.Name, reader.Value); + // Prefer: name, publicId, systemId, internalSubset + writer.WriteDocumentType(reader.Name, reader.PublicId, reader.SystemId, reader.InternalSubset);
219-220: Use theWriteStartElement(prefix, localName, namespaceUri)overload
Replacewriter.WriteStartElement(reader.Name);with
- writer.WriteStartElement(reader.Name); + var prefix = reader.Prefix; + var localName = reader.LocalName; + var nsUri = reader.NamespaceURI; + writer.WriteStartElement(prefix, localName, nsUri);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/OneScript.StandardLibrary/Xml/XmlReaderImpl.cs(2 hunks)src/OneScript.StandardLibrary/Xml/XmlReaderSettingsImpl.cs(2 hunks)src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs(1 hunks)tests/testrunner.os(2 hunks)tests/xmlwrite.os(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/xmlwrite.os
🧰 Additional context used
🧬 Code graph analysis (1)
src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs (2)
src/ScriptEngine/Machine/ValueFactory.cs (1)
ValueFactory(17-175)src/ScriptEngine/Machine/ValueAdoptionExtensions.cs (1)
ExplicitString(46-55)
🔇 Additional comments (1)
src/OneScript.StandardLibrary/Xml/XmlReaderSettingsImpl.cs (1)
103-111: Explicit DtdProcessing wiring — looks good.Using Ignore/Parse based on IgnoreDocumentType makes behavior clear and predictable.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
tests/xmlwrite.os (3)
367-455: Add a case for reserved prefix xml (e.g., xml:lang) to ensure no xmlns:xml is emitted.You already cover default and custom prefixes; add one test for xml:* to verify we don’t declare xmlns:xml and the prefix is preserved.
Would you like me to draft a tiny test that copies an attribute xml:lang="ru-RU" and asserts no xmlns:xml appears?
557-569: Add an assertion to the full-copy smoke test.Currently it only exercises the path. Assert that something meaningful is produced.
Apply this diff:
Процедура ТестДолжен_ЗаписатьТекущий_ПолноеКопирование() Экспорт @@ - ЗаписьXML.Закрыть(); + Результат = ЗаписьXML.Закрыть(); + юТест.Проверить(СтрДлина(Результат) > 0, "Копирование должно что-то записать"); КонецПроцедуры
571-593: Confirm intent: DOCTYPE copy ignores PUBLIC/SYSTEM.Test expects even when source has SYSTEM "example.dtd". If this is deliberate (security/simplicity), note it in docs; otherwise we should surface PUBLIC/SYSTEM.
I can wire a reader API probe and extend the writer to pass through public/system IDs if desired.
src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs (3)
186-228: Support processing instructions in WriteCurrent; keep strict errors for truly unsupported nodes.PI is a common node; we already expose WriteProcessingInstruction.
Apply this diff:
switch (nodeType) { + case XmlNodeType.ProcessingInstruction: + WriteProcessingInstruction(reader.Name, reader.Value); + break; @@ - case XmlNodeType.ProcessingInstruction: case XmlNodeType.Document: case XmlNodeType.DocumentFragment: case XmlNodeType.Notation: throw new RuntimeException(new Localization.BilingualString($"Копирование узла {nodeType} не поддерживается"));
280-286: C# naming nit: SplitName.Private helpers are typically PascalCase; also guard unusual multi-colon names.
Apply this diff and update callers:
-private static (string prefix, string localName) splitName(string nameOrLocalName) +private static (string prefix, string localName) SplitName(string nameOrLocalName) { - var parts = nameOrLocalName.Split(':'); - if (parts.Length > 1) return (parts[0], parts[1]); + var parts = nameOrLocalName.Split(':'); + if (parts.Length > 2) + return (parts[0], string.Join(":", parts.Skip(1))); + if (parts.Length > 1) return (parts[0], parts[1]); if (parts[0].Equals("xmlns", StringComparison.Ordinal)) return (parts[0], ""); return ("", parts[0]); }
230-234: DocumentType copies only the internal subset. Is dropping PUBLIC/SYSTEM intentional?If yes, consider a comment; if no, extend to pass through identifiers when available from the reader.
I can wire a reader-side accessor (publicId, systemId) and adjust WriteDocumentType accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs(1 hunks)tests/xmlwrite.os(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: dmpas
PR: EvilBeaver/OneScript#1583
File: src/OneScript.StandardLibrary/Xml/XmlReaderImpl.cs:389-397
Timestamp: 2025-09-11T05:07:24.790Z
Learning: In XmlReaderImpl.cs, the _reader.NodeType check in Read() method after ReadInternal() is not redundant because _reader.NodeType changes after every _reader.Read() call. The check inside ReadInternal() happens before reading, while the check in Read() happens after ReadInternal() completes, examining a potentially different node position.
📚 Learning: 2025-09-11T05:07:24.790Z
Learnt from: dmpas
PR: EvilBeaver/OneScript#1583
File: src/OneScript.StandardLibrary/Xml/XmlReaderImpl.cs:389-397
Timestamp: 2025-09-11T05:07:24.790Z
Learning: In XmlReaderImpl.cs, the _reader.NodeType check in Read() method after ReadInternal() is not redundant because _reader.NodeType changes after every _reader.Read() call. The check inside ReadInternal() happens before reading, while the check in Read() happens after ReadInternal() completes, examining a potentially different node position.
Applied to files:
src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs
🧬 Code graph analysis (1)
src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs (3)
src/ScriptEngine/Machine/ValueFactory.cs (1)
ValueFactory(17-175)src/ScriptEngine/Machine/ValueAdoptionExtensions.cs (1)
ExplicitString(46-55)src/OneScript.StandardLibrary/Xml/XmlNamespaceContext.cs (1)
XmlNamespaceContext(23-27)
🔇 Additional comments (2)
tests/xmlwrite.os (2)
29-44: Expanded WriteCurrent test coverage — nice.Good breadth across start/end element, attributes, namespaces, text/CDATA/comment, and decl/doctype.
486-556: Text/CDATA/Comment scenarios are well covered.Cases assert concatenation and repeatability correctly.
| private static void CopyAttribute(string attributeNameIn, string attributeValue, XmlReaderImpl reader, XmlWriterImpl writer) | ||
| { | ||
| var splittedName = splitName(attributeNameIn); | ||
| if (!string.IsNullOrEmpty(splittedName.prefix)) | ||
| { | ||
| var readerNsContext = (XmlNamespaceContext)reader.NamespaceContext; | ||
| var writerNsContext = (XmlNamespaceContext)writer.NamespaceContext; | ||
| if (splittedName.prefix.Equals("xmlns", StringComparison.Ordinal)) | ||
| { | ||
| writer.WriteNamespaceMapping(splittedName.localName, attributeValue); | ||
| return; | ||
| } | ||
| else | ||
| { | ||
| var uri = readerNsContext.LookupNamespaceUri(splittedName.prefix); | ||
| if (uri is BslStringValue) | ||
| { | ||
| var value = uri.ExplicitString(); | ||
| var curentPrefix = writerNsContext.LookupPrefix(value); | ||
| if (!(curentPrefix is BslStringValue && curentPrefix.ExplicitString().Equals(splittedName.prefix, StringComparison.Ordinal))) | ||
| { | ||
| writer.WriteNamespaceMapping(splittedName.prefix, value); | ||
| } | ||
| splittedName.prefix = value; | ||
| } | ||
| } | ||
| } | ||
| writer.WriteAttribute(splittedName.localName, splittedName.prefix, attributeValue); | ||
| } |
There was a problem hiding this comment.
Attributes: throw on unknown prefixes; fix variable typo; avoid declaring xmlns:xml.
Without a known URI, passing the prefix as ns leads to invalid output. Also rename curentPrefix -> currentPrefix.
Apply this diff:
- var splittedName = splitName(attributeNameIn);
+ var splittedName = SplitName(attributeNameIn);
@@
- var readerNsContext = (XmlNamespaceContext)reader.NamespaceContext;
- var writerNsContext = (XmlNamespaceContext)writer.NamespaceContext;
+ var readerNsContext = (XmlNamespaceContext)reader.NamespaceContext;
+ var writerNsContext = (XmlNamespaceContext)writer.NamespaceContext;
@@
- var uri = readerNsContext.LookupNamespaceUri(splittedName.prefix);
- if (uri is BslStringValue)
+ var uri = readerNsContext.LookupNamespaceUri(splittedName.prefix);
+ if (uri is BslStringValue)
{
var value = uri.ExplicitString();
- var curentPrefix = writerNsContext.LookupPrefix(value);
- if (!(curentPrefix is BslStringValue && curentPrefix.ExplicitString().Equals(splittedName.prefix, StringComparison.Ordinal)))
+ var currentPrefix = writerNsContext.LookupPrefix(value);
+ if (!(currentPrefix is BslStringValue && currentPrefix.ExplicitString().Equals(splittedName.prefix, StringComparison.Ordinal)))
{
- writer.WriteNamespaceMapping(splittedName.prefix, value);
+ if (!splittedName.prefix.Equals("xml", StringComparison.Ordinal))
+ writer.WriteNamespaceMapping(splittedName.prefix, value);
}
splittedName.prefix = value;
}
+ else
+ {
+ throw new RuntimeException(new Localization.BilingualString(
+ $"Неизвестный префикс пространства имён '{splittedName.prefix}' для атрибута '{attributeNameIn}'",
+ $"Unknown namespace prefix '{splittedName.prefix}' for attribute '{attributeNameIn}'"));
+ }
}
}
writer.WriteAttribute(splittedName.localName, splittedName.prefix, attributeValue);📝 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.
| private static void CopyAttribute(string attributeNameIn, string attributeValue, XmlReaderImpl reader, XmlWriterImpl writer) | |
| { | |
| var splittedName = splitName(attributeNameIn); | |
| if (!string.IsNullOrEmpty(splittedName.prefix)) | |
| { | |
| var readerNsContext = (XmlNamespaceContext)reader.NamespaceContext; | |
| var writerNsContext = (XmlNamespaceContext)writer.NamespaceContext; | |
| if (splittedName.prefix.Equals("xmlns", StringComparison.Ordinal)) | |
| { | |
| writer.WriteNamespaceMapping(splittedName.localName, attributeValue); | |
| return; | |
| } | |
| else | |
| { | |
| var uri = readerNsContext.LookupNamespaceUri(splittedName.prefix); | |
| if (uri is BslStringValue) | |
| { | |
| var value = uri.ExplicitString(); | |
| var curentPrefix = writerNsContext.LookupPrefix(value); | |
| if (!(curentPrefix is BslStringValue && curentPrefix.ExplicitString().Equals(splittedName.prefix, StringComparison.Ordinal))) | |
| { | |
| writer.WriteNamespaceMapping(splittedName.prefix, value); | |
| } | |
| splittedName.prefix = value; | |
| } | |
| } | |
| } | |
| writer.WriteAttribute(splittedName.localName, splittedName.prefix, attributeValue); | |
| } | |
| private static void CopyAttribute(string attributeNameIn, string attributeValue, XmlReaderImpl reader, XmlWriterImpl writer) | |
| { | |
| var splittedName = SplitName(attributeNameIn); | |
| if (!string.IsNullOrEmpty(splittedName.prefix)) | |
| { | |
| var readerNsContext = (XmlNamespaceContext)reader.NamespaceContext; | |
| var writerNsContext = (XmlNamespaceContext)writer.NamespaceContext; | |
| if (splittedName.prefix.Equals("xmlns", StringComparison.Ordinal)) | |
| { | |
| writer.WriteNamespaceMapping(splittedName.localName, attributeValue); | |
| return; | |
| } | |
| else | |
| { | |
| var uri = readerNsContext.LookupNamespaceUri(splittedName.prefix); | |
| if (uri is BslStringValue) | |
| { | |
| var value = uri.ExplicitString(); | |
| var currentPrefix = writerNsContext.LookupPrefix(value); | |
| if (!(currentPrefix is BslStringValue && currentPrefix.ExplicitString().Equals(splittedName.prefix, StringComparison.Ordinal))) | |
| { | |
| if (!splittedName.prefix.Equals("xml", StringComparison.Ordinal)) | |
| writer.WriteNamespaceMapping(splittedName.prefix, value); | |
| } | |
| splittedName.prefix = value; | |
| } | |
| else | |
| { | |
| throw new RuntimeException(new Localization.BilingualString( | |
| $"Неизвестный префикс пространства имён '{splittedName.prefix}' для атрибута '{attributeNameIn}'", | |
| $"Unknown namespace prefix '{splittedName.prefix}' for attribute '{attributeNameIn}'")); | |
| } | |
| } | |
| } | |
| writer.WriteAttribute(splittedName.localName, splittedName.prefix, attributeValue); | |
| } |
🤖 Prompt for AI Agents
In src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs around lines 250-278, the
CopyAttribute method must (1) throw an exception if a prefix is present but
LookupNamespaceUri returns null/empty (do not pass an unknown prefix as a
namespace), (2) skip writing a namespace mapping for the reserved "xml" prefix,
(3) rename the local variable curentPrefix to currentPrefix, and (4) only call
writer.WriteNamespaceMapping when a valid non-reserved namespace URI is found
and the current prefix in the writer differs; finally ensure splittedName.prefix
is set/left correctly (do not replace the prefix with the URI).
There was a problem hiding this comment.
passing the prefix as ns leads to invalid output
unresolved prefix as namespace - it's OK
fix variable typo
got it! thanks!
avoid declaring xmlns:xml
what's wrong with that?
There was a problem hiding this comment.
. The main concerns are:
- Redundancy - It's already implicitly bound, so explicit declaration adds no value
- Potential parser issues - Some XML processors might flag it as redundant or handle it inconsistently
- Best practices - The specification suggests it's not needed
So while your implementation might handle explicit xmlns:xml declarations correctly, skipping them aligns better with XML standards and reduces potential compatibility issues with other XML processors.
🧠 Learnings used
Learnt from: dmpas
PR: EvilBeaver/OneScript#1583
File: src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs:235-248
Timestamp: 2025-09-11T11:23:14.457Z
Learning: In XmlWriterImpl.cs WriteCurrent method, when copying elements using CopyElementAndAttributes, the WriteStartElement call should use reader.Name directly without preserving namespace/prefix information. This is expected behavior and differs from attribute handling which does preserve namespace context. Elements intentionally lose namespace info during copying.
Learnt from: dmpas
PR: EvilBeaver/OneScript#1583
File: src/OneScript.StandardLibrary/Xml/XmlReaderImpl.cs:389-397
Timestamp: 2025-09-11T05:07:24.790Z
Learning: In XmlReaderImpl.cs, the _reader.NodeType check in Read() method after ReadInternal() is not redundant because _reader.NodeType changes after every _reader.Read() call. The check inside ReadInternal() happens before reading, while the check in Read() happens after ReadInternal() completes, examining a potentially different node position.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs (5)
186-227: Consider supporting PI and entity refs in WriteCurrent; also make the error bilingual.PI and EntityReference can be copied trivially and you already expose writer methods. If intentionally unsupported, please confirm and add tests to assert throws. Also make the exception bilingual for consistency.
- case XmlNodeType.Comment: - WriteComment(reader.Value); - break; + case XmlNodeType.Comment: + WriteComment(reader.Value); + break; + case XmlNodeType.ProcessingInstruction: + WriteProcessingInstruction(reader.Name, reader.Value); + break; + case XmlNodeType.EntityReference: + WriteEntityReference(reader.Name); + break; @@ - case XmlNodeType.Entity: - case XmlNodeType.EntityReference: - case XmlNodeType.EndEntity: - case XmlNodeType.ProcessingInstruction: + case XmlNodeType.Entity: + case XmlNodeType.EndEntity: case XmlNodeType.Document: case XmlNodeType.DocumentFragment: case XmlNodeType.Notation: - throw new RuntimeException(new Localization.BilingualString($"Копирование узла {nodeType} не поддерживается")); + throw new RuntimeException(new Localization.BilingualString( + $"Копирование узла {nodeType} не поддерживается", + $"Copying node {nodeType} is not supported"));
230-234: Avoid emitting an empty internal subset as “[]”; treat empty as null (and consider PUBLIC/SYSTEM later).When DocumentType has no internal subset, reader.Value can be empty. Passing empty string may render “[]”. Pass null instead. If XmlReaderImpl exposes PUBLIC/SYSTEM later, prefer preserving them too.
- writer.WriteDocumentType(reader.Name, reader.Value); + var subset = reader.Value; + writer.WriteDocumentType(reader.Name, string.IsNullOrEmpty(subset) ? null : subset);
250-278: Minor naming/style nits; document the “unknown prefix → use as ns” fallback.
- Use PascalCase for method names: splitName → SplitName.
- Variable “splittedName” reads awkward; consider qname/nameParts.
- Optional: skip emitting xmlns:xml mapping to reduce noise (it’s predeclared), but OK if you keep it.
Please also add a brief comment noting that if the prefix is unknown, it’s intentionally treated as a namespace URI (as discussed), so future contributors don’t “fix” it.
- var splittedName = splitName(attributeNameIn); + var qname = SplitName(attributeNameIn); - if (!string.IsNullOrEmpty(splittedName.prefix)) + if (!string.IsNullOrEmpty(qname.prefix)) { var readerNsContext = (XmlNamespaceContext)reader.NamespaceContext; var writerNsContext = (XmlNamespaceContext)writer.NamespaceContext; - if (splittedName.prefix.Equals("xmlns", StringComparison.Ordinal)) + if (qname.prefix.Equals("xmlns", StringComparison.Ordinal)) { - writer.WriteNamespaceMapping(splittedName.localName, attributeValue); + writer.WriteNamespaceMapping(qname.localName, attributeValue); return; } else { - var uri = readerNsContext.LookupNamespaceUri(splittedName.prefix); + var uri = readerNsContext.LookupNamespaceUri(qname.prefix); if (uri is BslStringValue) { var value = uri.ExplicitString(); var currentPrefix = writerNsContext.LookupPrefix(value); - if (!(currentPrefix is BslStringValue && currentPrefix.ExplicitString().Equals(splittedName.prefix, StringComparison.Ordinal))) + if (!(currentPrefix is BslStringValue && currentPrefix.ExplicitString().Equals(qname.prefix, StringComparison.Ordinal))) { - writer.WriteNamespaceMapping(splittedName.prefix, value); + writer.WriteNamespaceMapping(qname.prefix, value); } - splittedName.prefix = value; + qname.prefix = value; // use URI for writer API } } } - writer.WriteAttribute(splittedName.localName, splittedName.prefix, attributeValue); + writer.WriteAttribute(qname.localName, qname.prefix, attributeValue);
280-286: Rename splitName → SplitName; small alloc-free variant.PascalCase for methods; also avoid string.Split to cut allocs in tight loops.
- private static (string prefix, string localName) splitName(string nameOrLocalName) + private static (string prefix, string localName) SplitName(string nameOrLocalName) { - var parts = nameOrLocalName.Split(':'); - if (parts.Length > 1) return (parts[0], parts[1]); - if (parts[0].Equals("xmlns", StringComparison.Ordinal)) return (parts[0], ""); - return ("", parts[0]); + var idx = nameOrLocalName.IndexOf(':'); + if (idx > 0) return (nameOrLocalName.Substring(0, idx), nameOrLocalName.Substring(idx + 1)); + if (nameOrLocalName.Equals("xmlns", StringComparison.Ordinal)) return ("xmlns", ""); + return ("", nameOrLocalName); }
183-228: Add tests for uncovered/edge cases in WriteCurrent.Targeted cases:
- ProcessingInstruction and EntityReference: either copied or assert throws (current behavior).
- DocumentType with PUBLIC/SYSTEM ids and with empty internal subset (no “[]”).
- Attributes: unknown prefix fallback, xml:lang/xml:space, default ns vs. prefixed ns on attributes.
- Ensure correct behavior when element ns is intentionally not preserved (noting prior decision).
I can draft these tests against tests/xmlwrite.os and testrunner.os. Want me to open a checklist PR task and push a spec?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: dmpas
PR: EvilBeaver/OneScript#1583
File: src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs:235-248
Timestamp: 2025-09-11T11:23:14.457Z
Learning: In XmlWriterImpl.cs WriteCurrent method, when copying elements using CopyElementAndAttributes, the WriteStartElement call should use reader.Name directly without preserving namespace/prefix information. This is expected behavior and differs from attribute handling which does preserve namespace context. Elements intentionally lose namespace info during copying.
Learnt from: dmpas
PR: EvilBeaver/OneScript#1583
File: src/OneScript.StandardLibrary/Xml/XmlReaderImpl.cs:389-397
Timestamp: 2025-09-11T05:07:24.790Z
Learning: In XmlReaderImpl.cs, the _reader.NodeType check in Read() method after ReadInternal() is not redundant because _reader.NodeType changes after every _reader.Read() call. The check inside ReadInternal() happens before reading, while the check in Read() happens after ReadInternal() completes, examining a potentially different node position.
📚 Learning: 2025-09-11T11:23:14.457Z
Learnt from: dmpas
PR: EvilBeaver/OneScript#1583
File: src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs:235-248
Timestamp: 2025-09-11T11:23:14.457Z
Learning: In XmlWriterImpl.cs WriteCurrent method, when copying elements using CopyElementAndAttributes, the WriteStartElement call should use reader.Name directly without preserving namespace/prefix information. This is expected behavior and differs from attribute handling which does preserve namespace context. Elements intentionally lose namespace info during copying.
Applied to files:
src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs
📚 Learning: 2025-09-11T05:07:24.790Z
Learnt from: dmpas
PR: EvilBeaver/OneScript#1583
File: src/OneScript.StandardLibrary/Xml/XmlReaderImpl.cs:389-397
Timestamp: 2025-09-11T05:07:24.790Z
Learning: In XmlReaderImpl.cs, the _reader.NodeType check in Read() method after ReadInternal() is not redundant because _reader.NodeType changes after every _reader.Read() call. The check inside ReadInternal() happens before reading, while the check in Read() happens after ReadInternal() completes, examining a potentially different node position.
Applied to files:
src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs
🧬 Code graph analysis (1)
src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs (3)
src/ScriptEngine/Machine/ValueFactory.cs (1)
ValueFactory(17-175)src/ScriptEngine/Machine/ValueAdoptionExtensions.cs (1)
ExplicitString(46-55)src/OneScript.StandardLibrary/Xml/XmlNamespaceContext.cs (1)
XmlNamespaceContext(23-27)
🔇 Additional comments (1)
src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs (1)
235-248: LGTM: Start element via reader.Name aligns with intended behavior.Per prior clarification, dropping element namespace/prefix is expected; attribute logic preserves ns context. This implementation matches that contract.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs (1)
239-252: Start element copying: using reader.Name is intentional.Using
WriteStartElement(reader.Name)(without preserving ns/prefix) aligns with the agreed behavior for elements; attributes remain namespace‑aware. No change requested.Note: Based on the prior discussion and stored learning, elements intentionally lose namespace info during copying, unlike attributes.
🧹 Nitpick comments (6)
src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs (3)
234-237: DocumentType: confirm intent to drop PUBLIC/SYSTEM IDs.
CopyDocumentTypeemits onlyname+ internal subset (reader.Value) viaWriteDocType(name, null, null, subset). This intentionally ignores PUBLIC/SYSTEM identifiers from the source. If that’s by design, add a short comment here to prevent future “reintroduce IDs” regressions; otherwise, we can plumb PublicId/SystemId when available.private static void CopyDocumentType(XmlReaderImpl reader, XmlWriterImpl writer) { - writer.WriteDocumentType(reader.Name, reader.Value); + // Intentionally write only the current node payload (internal subset), + // not PUBLIC/SYSTEM ids. See tests: _ОбъявлениеИТипДокумента. + writer.WriteDocumentType(reader.Name, reader.Value); }
254-282: Attributes: skip declaring the reserved ‘xml’ prefix; small naming/clarity nits.
- Don’t emit
xmlns:xml: thexmlprefix is predefined and must not be redeclared.- Consider not overloading
prefixto hold a URI — use a localnsUrito avoid confusion.- Minor:
splitName/splittedNamenaming.- var splittedName = splitName(attributeNameIn); + var splittedName = SplitName(attributeNameIn); @@ - if (splittedName.prefix.Equals("xmlns", StringComparison.Ordinal)) + if (splittedName.prefix.Equals("xmlns", StringComparison.Ordinal)) { - writer.WriteNamespaceMapping(splittedName.localName, attributeValue); + // Skip redeclaring the reserved 'xml' prefix. + if (!splittedName.localName.Equals("xml", StringComparison.Ordinal)) + writer.WriteNamespaceMapping(splittedName.localName, attributeValue); return; } else { var uri = readerNsContext.LookupNamespaceUri(splittedName.prefix); if (uri is BslStringValue) { - var value = uri.ExplicitString(); - var currentPrefix = writerNsContext.LookupPrefix(value); + var nsUri = uri.ExplicitString(); + var currentPrefix = writerNsContext.LookupPrefix(nsUri); if (!(currentPrefix is BslStringValue && currentPrefix.ExplicitString().Equals(splittedName.prefix, StringComparison.Ordinal))) { - writer.WriteNamespaceMapping(splittedName.prefix, value); + if (!splittedName.prefix.Equals("xml", StringComparison.Ordinal)) + writer.WriteNamespaceMapping(splittedName.prefix, nsUri); } - splittedName.prefix = value; + // Use the namespace URI when writing the attribute. + splittedName.prefix = nsUri; } } } writer.WriteAttribute(splittedName.localName, splittedName.prefix, attributeValue);Please add a focused test to assert that copying an
xml:space="preserve"attribute does not emitxmlns:xml. Also, a test for an attribute with an unknown prefix would help lock in the current “treat prefix as namespace URI” behavior (if that’s intentional).
284-290: Rename helper for consistency; optionally validate illegal names.
- Prefer
SplitName(PascalCase) to match local conventions.- Optional: if multiple ‘:’ are present, consider throwing to guard illegal QNames.
- private static (string prefix, string localName) splitName(string nameOrLocalName) + private static (string prefix, string localName) SplitName(string nameOrLocalName) { var parts = nameOrLocalName.Split(':'); - if (parts.Length > 1) return (parts[0], parts[1]); + if (parts.Length > 1) return (parts[0], parts[1]); // assume exactly one ':' if (parts[0].Equals("xmlns", StringComparison.Ordinal)) return (parts[0], ""); return ("", parts[0]); }tests/xmlwrite.os (3)
30-46: Fix test name typo: “Сушность” → “Сущность”.For clarity and grep-ability, align the test list entry and procedure name.
- ВсеТесты.Добавить("ТестДолжен_ЗаписатьТекущий_СсылкаНаСушность"); + ВсеТесты.Добавить("ТестДолжен_ЗаписатьТекущий_СсылкаНаСущность"); @@ -Процедура ТестДолжен_ЗаписатьТекущий_СсылкаНаСушность() Экспорт +Процедура ТестДолжен_ЗаписатьТекущий_СсылкаНаСущность() ЭкспортAlso applies to: 596-633
559-571: Consider asserting the full-copy case for regression safety.
ТестДолжен_ЗаписатьТекущий_ПолноеКопированиеcurrently only ensures “no exceptions”. If feasible, assert a normalized expected output to catch future regressions in node handling.
488-509: Minor message text nit.Diagnostic message mentions “Some text” while the value is “Some Text”. Not user-facing, but easy to align.
- ПрочитатьИПроверитьТип(ЧтениеXML, ТипУзлаXML.Текст, "Спозиционированы на текст Some text"); + ПрочитатьИПроверитьТип(ЧтениеXML, ТипУзлаXML.Текст, "Спозиционированы на текст Some Text");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs(1 hunks)tests/xmlwrite.os(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: dmpas
PR: EvilBeaver/OneScript#1583
File: src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs:235-248
Timestamp: 2025-09-11T11:23:14.457Z
Learning: In XmlWriterImpl.cs WriteCurrent method, when copying elements using CopyElementAndAttributes, the WriteStartElement call should use reader.Name directly without preserving namespace/prefix information. This is expected behavior and differs from attribute handling which does preserve namespace context. Elements intentionally lose namespace info during copying.
Learnt from: dmpas
PR: EvilBeaver/OneScript#1583
File: src/OneScript.StandardLibrary/Xml/XmlReaderImpl.cs:389-397
Timestamp: 2025-09-11T05:07:24.790Z
Learning: In XmlReaderImpl.cs, the _reader.NodeType check in Read() method after ReadInternal() is not redundant because _reader.NodeType changes after every _reader.Read() call. The check inside ReadInternal() happens before reading, while the check in Read() happens after ReadInternal() completes, examining a potentially different node position.
📚 Learning: 2025-09-11T11:23:14.457Z
Learnt from: dmpas
PR: EvilBeaver/OneScript#1583
File: src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs:235-248
Timestamp: 2025-09-11T11:23:14.457Z
Learning: In XmlWriterImpl.cs WriteCurrent method, when copying elements using CopyElementAndAttributes, the WriteStartElement call should use reader.Name directly without preserving namespace/prefix information. This is expected behavior and differs from attribute handling which does preserve namespace context. Elements intentionally lose namespace info during copying.
Applied to files:
src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs
📚 Learning: 2025-09-11T05:07:24.790Z
Learnt from: dmpas
PR: EvilBeaver/OneScript#1583
File: src/OneScript.StandardLibrary/Xml/XmlReaderImpl.cs:389-397
Timestamp: 2025-09-11T05:07:24.790Z
Learning: In XmlReaderImpl.cs, the _reader.NodeType check in Read() method after ReadInternal() is not redundant because _reader.NodeType changes after every _reader.Read() call. The check inside ReadInternal() happens before reading, while the check in Read() happens after ReadInternal() completes, examining a potentially different node position.
Applied to files:
src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs
🧬 Code graph analysis (1)
src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs (3)
src/ScriptEngine/Machine/ValueFactory.cs (1)
ValueFactory(17-175)src/ScriptEngine/Machine/ValueAdoptionExtensions.cs (1)
ExplicitString(46-55)src/OneScript.StandardLibrary/Xml/XmlNamespaceContext.cs (1)
XmlNamespaceContext(23-27)
🔇 Additional comments (4)
src/OneScript.StandardLibrary/Xml/XmlWriterImpl.cs (1)
186-231: Node-type switch is clear and matches “write only current node” intent.Covers the expected cases, with unsupported ones failing fast. LGTM.
tests/xmlwrite.os (3)
573-593: Nice: declaration + doctype expectations captured case-insensitively.The assertion reflects platform quirks (subset formatting). LGTM.
370-395: Great coverage for namespace redeclare/override scenarios.The three tests around attribute namespace mapping (define, redundant, and prefix-mismatch) read well and lock intended behavior. LGTM.
Also applies to: 427-456
307-323: Element-start cases: expectations match the “elements lose ns info, attributes keep it” model.Tests are precise and helpful to prevent accidental reversion. LGTM.
Also applies to: 325-343
dmpas
changed the title
|
@dmpas WIP снят - можно вливать? |
Можно. |
2 participants
Summary by CodeRabbit
Bug Fixes
Tests
Tests/Tools