You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucenenet.apache.org by "laimis (via GitHub)" <gi...@apache.org> on 2023/04/13 21:40:09 UTC
[GitHub] [lucenenet] laimis opened a new pull request, #835: Virtual method call from the constructor fixes for ElementType
laimis opened a new pull request, #835:
URL: https://github.com/apache/lucenenet/pull/835
Continuation of fixes with virtual calls being made from constructors. The issue originally reported by SonarCloud scans: https://sonarcloud.io/project/issues?resolved=false&rules=csharpsquid%3AS1699&id=apache_lucenenet and referenced in this issue: https://github.com/apache/lucenenet/issues/670
This one focuses on ElementType. We are suppressing the warnings and including docs with a note for subclasses to use a different constructor if needed.
--
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: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] laimis commented on a diff in pull request #835: Virtual method call from the constructor fixes for ElementType
Posted by "laimis (via GitHub)" <gi...@apache.org>.
laimis commented on code in PR #835:
URL: https://github.com/apache/lucenenet/pull/835#discussion_r1167022722
##########
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:
That was a copy paste mess up, fixed it
--
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: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] NightOwl888 commented on a diff in pull request #835: Virtual method call from the constructor fixes for ElementType
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] laimis commented on a diff in pull request #835: Virtual method call from the constructor fixes for ElementType
Posted by "laimis (via GitHub)" <gi...@apache.org>.
laimis commented on code in PR #835:
URL: https://github.com/apache/lucenenet/pull/835#discussion_r1167026651
##########
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:
Adjusted the constructor to accept namespace and localName strings vs funcs. I am hesitant to get into big refactoring and focus on getting the sonar warnings eliminated first, so I didn't touch the rest.
--
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: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] laimis merged pull request #835: Virtual method call from the constructor fixes for ElementType
Posted by "laimis (via GitHub)" <gi...@apache.org>.
laimis merged PR #835:
URL: https://github.com/apache/lucenenet/pull/835
--
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: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org