[GitHub] [solr] dsmiley commented on a change in pull request #165: SOLR-15385 RawTypes Part II, Searching for NamedList

[GitBox <gi...@apache.org>]

dsmiley commented on a change in pull request #165:
URL: https://github.com/apache/solr/pull/165#discussion_r645937456



##########
File path: solr/core/src/java/org/apache/solr/util/plugin/NamedListInitializedPlugin.java
##########
@@ -25,5 +25,10 @@
  * @since solr 1.3
  */
 public interface NamedListInitializedPlugin {
-  void init( @SuppressWarnings({"rawtypes"})NamedList args );
+  /**
+   * <code>init</code> will be called just once, immediately after creation.
+   * <p>The args are user-level initialization parameters that

Review comment:
       What is a "user-level initialization parameter"?

##########
File path: solr/core/src/java/org/apache/solr/util/plugin/NamedListInitializedPlugin.java
##########
@@ -25,5 +25,10 @@
  * @since solr 1.3
  */
 public interface NamedListInitializedPlugin {
-  void init( @SuppressWarnings({"rawtypes"})NamedList args );
+  /**
+   * <code>init</code> will be called just once, immediately after creation.
+   * <p>The args are user-level initialization parameters that
+   * may be specified, usually in solrconfig.xml
+   */
+  default void init(NamedList<?> args) {}

Review comment:
       Today, NamedListInitializedPlugin is implemented by plugin types (e.g. SearchComponent) and thus I can see why you are putting a default impl here.  But I wonder if it would be better to treat this interface as an opt-in thing that a specific plugin can implement if it has something to initialize, but otherwise not?  Granted if the base type has some default/common initialization, then in that case, the plugin type would continue to implement this interface, but most don't need to, I think.  Any way, this would be out of scope of this PR but I welcome your opinion nonetheless.

##########
File path: solr/core/src/java/org/apache/solr/filestore/DistribPackageStore.java
##########
@@ -523,6 +523,8 @@ public void refresh(String path) {
         @SuppressWarnings({"rawtypes"})
         List myFiles = list(path, s -> true);
         for (Object f : l) {
+          // XXX DUBIOUS XXX

Review comment:
       Maybe use "TODO"; I'm unaware of "XXX" practice

##########
File path: solr/core/src/java/org/apache/solr/util/plugin/NamedListInitializedPlugin.java
##########
@@ -25,5 +25,10 @@
  * @since solr 1.3
  */
 public interface NamedListInitializedPlugin {
-  void init( @SuppressWarnings({"rawtypes"})NamedList args );
+  /**
+   * <code>init</code> will be called just once, immediately after creation.
+   * <p>The args are user-level initialization parameters that
+   * may be specified, usually in solrconfig.xml

Review comment:
       say but it depends on the plugin type.

##########
File path: solr/core/src/java/org/apache/solr/api/ApiBag.java
##########
@@ -301,19 +301,18 @@ public static SpecProvider constructSpec(PluginInfo info) {
     Object specObj = info == null ? null : info.attributes.get("spec");
     if (specObj == null) specObj = "emptySpec";
     if (specObj instanceof Map) {
-      @SuppressWarnings({"rawtypes"})
-      Map map = (Map) specObj;
+        assert false : "got a map from a string";

Review comment:
       Why this?

##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -204,9 +204,9 @@ public final ConfigSet loadConfigSet(CoreDescriptor dcore) {
     try {
 
       // ConfigSet properties are loaded from ConfigSetProperties.DEFAULT_FILENAME file.
-      NamedList properties = loadConfigSetProperties(dcore, coreLoader);
+      NamedList<Object> properties = loadConfigSetProperties(dcore, coreLoader);

Review comment:
       Curious; why not "?" here like you did a couple lines below for flags?

##########
File path: solr/core/src/test/org/apache/solr/spelling/FileBasedSpellCheckerTest.java
##########
@@ -143,19 +137,16 @@ public void testFieldType() throws Exception {
    * No indexDir location set
    */
   @Test
-  @SuppressWarnings({"unchecked"})
   public void testRAMDirectory() throws Exception {
     FileBasedSpellChecker checker = new FileBasedSpellChecker();
-    @SuppressWarnings({"rawtypes"})
-    NamedList spellchecker = new NamedList();
+    NamedList<Object> spellchecker = new NamedList<>();
     spellchecker.add("classname", FileBasedSpellChecker.class.getName());
 
     spellchecker.add(SolrSpellChecker.DICTIONARY_NAME, "external");
     spellchecker.add(AbstractLuceneSpellChecker.LOCATION, "spellings.txt");
     spellchecker.add(FileBasedSpellChecker.SOURCE_FILE_CHAR_ENCODING, "UTF-8");
     spellchecker.add(AbstractLuceneSpellChecker.FIELD, "teststop");
     spellchecker.add(SolrSpellChecker.FIELD_TYPE, "teststop");
-    spellchecker.add(AbstractLuceneSpellChecker.SPELLCHECKER_ARG_NAME, spellchecker);

Review comment:
       why remove?

##########
File path: solr/core/src/java/org/apache/solr/util/plugin/NamedListInitializedPlugin.java
##########
@@ -25,5 +25,10 @@
  * @since solr 1.3
  */
 public interface NamedListInitializedPlugin {
-  void init( @SuppressWarnings({"rawtypes"})NamedList args );
+  /**
+   * <code>init</code> will be called just once, immediately after creation.

Review comment:
       Nice to see you adding some docs here.  Let's also state that "args" is never null?




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org