NightOwl888 commented on code in PR #835:
URL: https://github.com/apache/lucenenet/pull/835#discussion_r1166062576
##########
src/Lucene.Net.Benchmark/Support/TagSoup/ElementType.cs:
##########
@@ -59,6 +62,26 @@ public ElementType(string name, int model, int memberOf, int
flags, Schema schem
localName = GetLocalName(name);
}
+ /// <summary>
+ /// LUCENENET specific constructor that allows the caller to specify
the namespace and local name
+ /// and is provided to subclasses as an alternative to <see
cref="ElementType(string, int, int, int, Schema)"/>
+ /// in order to avoid virtual method calls.
+ /// </summary>
+ public ElementType(
+ string name, int model, int memberOf, int flags, Schema schema,
+ Func<string, bool, string> namespaceFunc,
Review Comment:
I think a better route to take here would be to allow the `namespace` and
`localName` to be passed in.
```c#
public ElementType(string name, string @namespace, string localName, int
model, int memberOf, int flags, Schema schema)
```
And then mention in the docs that these can be retrieved from `name` by
calling `GetNamespace(name, false)` and `GetLocalName(name)`.
Do note that Microsoft names these a bit differently - `prefix`,
`localName`, and `namespaceURI`:
https://learn.microsoft.com/en-us/dotnet/api/system.xml.xmldocument.createattribute?view=net-7.0#system-xml-xmldocument-createattribute(system-string-system-string-system-string).
Perhaps it would be worth it to update all of TagSoup and SAX to be
consistent with these names and in this order. I noticed SAX uses `GetURI` for
"namespace URI" and TagSoup uses `@namespace`, which conflicts with the C#
`namespace` keyword.
Do be aware that TagSoup has a T4 template named `HTMLSchema.tt` that
auto-generates some code based on some XSLT templates and data files, so we
should check whether these changes affect that generated code. Also, I have no
idea whether it will generate the identical code that we have checked in -
perhaps we should check that.
##########
src/Lucene.Net.Benchmark/Support/TagSoup/ElementType.cs:
##########
@@ -47,6 +48,8 @@ public class ElementType
/// <param name="schema">
/// The schema with which this element type will be associated
/// </param>
+ [SuppressMessage("CodeQuality", "IDE0079:Remove unnecessary
suppression", Justification = "This is a SonarCloud issue")]
+ [SuppressMessage("CodeQuality", "S1699:Constructors should only call
non-overridable methods", Justification = "This class gets deprecated and
removed in later versions")]
Review Comment:
When you say "later versions", are you referring to TagSoup or Lucene?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
