You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucene.apache.org by "David Smiley (Created) (JIRA)" <ji...@apache.org> on 2011/11/25 22:11:40 UTC
[jira] [Created] (SOLR-2920) Refactor frequent conditional use of
DefaultSolrParams into SolrParams.combine(p,d)
Refactor frequent conditional use of DefaultSolrParams into SolrParams.combine(p,d)
-----------------------------------------------------------------------------------
Key: SOLR-2920
URL: https://issues.apache.org/jira/browse/SOLR-2920
Project: Solr
Issue Type: Improvement
Reporter: David Smiley
Priority: Minor
Attachments: SOLR-2920_SolrParams_combine().patch
I had a small bug in use of DefaultSolrParams in my code because I didn't check for non-existent defaults. I noticed through the Solr codebase that is code pattern is very common:
{code:java}
if( defaults != null ) {
params = new DefaultSolrParams( params, defaults );
}
{code}
Instead, I refactored this logic into a new SolrParams.combine(p,d) method and made it so that nobody refers to DefaultSolrParams. I did similarly for AppendedSolrParams.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[jira] [Commented] (SOLR-2920) Refactor frequent conditional use of
DefaultSolrParams into SolrParams.combine(p,d)
Posted by "Hoss Man (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-2920?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13166462#comment-13166462 ]
Hoss Man commented on SOLR-2920:
--------------------------------
David: thanks for the updated, reading the diff something wonky jumped out at me though...
in SingleFragListBuilder & SimpleFragListBuilder the "params" are no longer wrapped in defaults at all - is that intentional? was that dead code before? .. or is that just a accidental deletion?
> Refactor frequent conditional use of DefaultSolrParams into SolrParams.combine(p,d)
> -----------------------------------------------------------------------------------
>
> Key: SOLR-2920
> URL: https://issues.apache.org/jira/browse/SOLR-2920
> Project: Solr
> Issue Type: Improvement
> Reporter: David Smiley
> Priority: Minor
> Attachments: SOLR-2920_SolrParams_combine().patch, SOLR-2920_SolrParams_wrapDefaults_and_wrapAppended.patch
>
>
> I had a small bug in use of DefaultSolrParams in my code because I didn't check for non-existent defaults. I noticed through the Solr codebase that is code pattern is very common:
> {code:java}
> if( defaults != null ) {
> params = new DefaultSolrParams( params, defaults );
> }
> {code}
> Instead, I refactored this logic into a new SolrParams.combine(p,d) method and made it so that nobody refers to DefaultSolrParams. I did similarly for AppendedSolrParams.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[jira] [Commented] (SOLR-2920) Refactor frequent conditional use of
DefaultSolrParams into SolrParams.combine(p,d)
Posted by "David Smiley (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-2920?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13163878#comment-13163878 ]
David Smiley commented on SOLR-2920:
------------------------------------
Anybody have any opinion here? I'd love to get this issue to a resolution.
> Refactor frequent conditional use of DefaultSolrParams into SolrParams.combine(p,d)
> -----------------------------------------------------------------------------------
>
> Key: SOLR-2920
> URL: https://issues.apache.org/jira/browse/SOLR-2920
> Project: Solr
> Issue Type: Improvement
> Reporter: David Smiley
> Priority: Minor
> Attachments: SOLR-2920_SolrParams_combine().patch
>
>
> I had a small bug in use of DefaultSolrParams in my code because I didn't check for non-existent defaults. I noticed through the Solr codebase that is code pattern is very common:
> {code:java}
> if( defaults != null ) {
> params = new DefaultSolrParams( params, defaults );
> }
> {code}
> Instead, I refactored this logic into a new SolrParams.combine(p,d) method and made it so that nobody refers to DefaultSolrParams. I did similarly for AppendedSolrParams.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[jira] [Resolved] (SOLR-2920) Refactor frequent conditional use of
DefaultSolrParams into SolrParams.combine(p,d)
Posted by "Hoss Man (Resolved) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-2920?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Hoss Man resolved SOLR-2920.
----------------------------
Resolution: Fixed
Fix Version/s: 4.0
3.6
Assignee: David Smiley
Committed revision 1213484. - 3x
David: thanks for the suggestion & the following through with patch!
> Refactor frequent conditional use of DefaultSolrParams into SolrParams.combine(p,d)
> -----------------------------------------------------------------------------------
>
> Key: SOLR-2920
> URL: https://issues.apache.org/jira/browse/SOLR-2920
> Project: Solr
> Issue Type: Improvement
> Reporter: David Smiley
> Assignee: David Smiley
> Priority: Minor
> Fix For: 3.6, 4.0
>
> Attachments: SOLR-2920_SolrParams_combine().patch, SOLR-2920_SolrParams_wrapDefaults_and_wrapAppended.patch
>
>
> I had a small bug in use of DefaultSolrParams in my code because I didn't check for non-existent defaults. I noticed through the Solr codebase that is code pattern is very common:
> {code:java}
> if( defaults != null ) {
> params = new DefaultSolrParams( params, defaults );
> }
> {code}
> Instead, I refactored this logic into a new SolrParams.combine(p,d) method and made it so that nobody refers to DefaultSolrParams. I did similarly for AppendedSolrParams.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[jira] [Commented] (SOLR-2920) Refactor frequent conditional use of
DefaultSolrParams into SolrParams.combine(p,d)
Posted by "David Smiley (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-2920?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13159850#comment-13159850 ]
David Smiley commented on SOLR-2920:
------------------------------------
Thanks for noticing my issue, Hoss.
"combine" vs "merge" seem similar to me. "wrap" (if on DefaultSolrParams) or "wrapDefaults" (if not) sounds clearer to me.
I started this by putting a method in the respective class it came from, but then I noticed that SolrParams already has some handy static utility methods. Then it occurred to me that it would be convenient if the caller didn't need to know about all the classes in this package -- just SolrParams with some static factory methods. It's just one class to import too. Extending this line of reasoning suggests more refactoring would be in order to create a MapSolrParams. What do you think?
RE import statements: It would be nice if there was a standard the project could agree on so we could just configure our IDEs consistently. In the mean time, I'll try to reduce import statement modifications.
> Refactor frequent conditional use of DefaultSolrParams into SolrParams.combine(p,d)
> -----------------------------------------------------------------------------------
>
> Key: SOLR-2920
> URL: https://issues.apache.org/jira/browse/SOLR-2920
> Project: Solr
> Issue Type: Improvement
> Reporter: David Smiley
> Priority: Minor
> Attachments: SOLR-2920_SolrParams_combine().patch
>
>
> I had a small bug in use of DefaultSolrParams in my code because I didn't check for non-existent defaults. I noticed through the Solr codebase that is code pattern is very common:
> {code:java}
> if( defaults != null ) {
> params = new DefaultSolrParams( params, defaults );
> }
> {code}
> Instead, I refactored this logic into a new SolrParams.combine(p,d) method and made it so that nobody refers to DefaultSolrParams. I did similarly for AppendedSolrParams.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[jira] [Commented] (SOLR-2920) Refactor frequent conditional use of
DefaultSolrParams into SolrParams.combine(p,d)
Posted by "Hoss Man (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-2920?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13167889#comment-13167889 ]
Hoss Man commented on SOLR-2920:
--------------------------------
yep yep ... totally get it now. they don't use any of the params they are given, so no reason to waste the cycles wrapping the defaults.
just in case though, i added a comment about the defaults in case those methods ever _do_ start using defaults.
Committed revision 1213474. - trunk
...testing the merge back to 3x now.
> Refactor frequent conditional use of DefaultSolrParams into SolrParams.combine(p,d)
> -----------------------------------------------------------------------------------
>
> Key: SOLR-2920
> URL: https://issues.apache.org/jira/browse/SOLR-2920
> Project: Solr
> Issue Type: Improvement
> Reporter: David Smiley
> Priority: Minor
> Attachments: SOLR-2920_SolrParams_combine().patch, SOLR-2920_SolrParams_wrapDefaults_and_wrapAppended.patch
>
>
> I had a small bug in use of DefaultSolrParams in my code because I didn't check for non-existent defaults. I noticed through the Solr codebase that is code pattern is very common:
> {code:java}
> if( defaults != null ) {
> params = new DefaultSolrParams( params, defaults );
> }
> {code}
> Instead, I refactored this logic into a new SolrParams.combine(p,d) method and made it so that nobody refers to DefaultSolrParams. I did similarly for AppendedSolrParams.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[jira] [Updated] (SOLR-2920) Refactor frequent conditional use of
DefaultSolrParams into SolrParams.combine(p,d)
Posted by "David Smiley (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-2920?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
David Smiley updated SOLR-2920:
-------------------------------
Attachment: SOLR-2920_SolrParams_combine().patch
The attached patch was done against trunk r1205366
> Refactor frequent conditional use of DefaultSolrParams into SolrParams.combine(p,d)
> -----------------------------------------------------------------------------------
>
> Key: SOLR-2920
> URL: https://issues.apache.org/jira/browse/SOLR-2920
> Project: Solr
> Issue Type: Improvement
> Reporter: David Smiley
> Priority: Minor
> Attachments: SOLR-2920_SolrParams_combine().patch
>
>
> I had a small bug in use of DefaultSolrParams in my code because I didn't check for non-existent defaults. I noticed through the Solr codebase that is code pattern is very common:
> {code:java}
> if( defaults != null ) {
> params = new DefaultSolrParams( params, defaults );
> }
> {code}
> Instead, I refactored this logic into a new SolrParams.combine(p,d) method and made it so that nobody refers to DefaultSolrParams. I did similarly for AppendedSolrParams.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[jira] [Commented] (SOLR-2920) Refactor frequent conditional use of
DefaultSolrParams into SolrParams.combine(p,d)
Posted by "David Smiley (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-2920?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13166635#comment-13166635 ]
David Smiley commented on SOLR-2920:
------------------------------------
Those params weren't used at all -- dead code.
> Refactor frequent conditional use of DefaultSolrParams into SolrParams.combine(p,d)
> -----------------------------------------------------------------------------------
>
> Key: SOLR-2920
> URL: https://issues.apache.org/jira/browse/SOLR-2920
> Project: Solr
> Issue Type: Improvement
> Reporter: David Smiley
> Priority: Minor
> Attachments: SOLR-2920_SolrParams_combine().patch, SOLR-2920_SolrParams_wrapDefaults_and_wrapAppended.patch
>
>
> I had a small bug in use of DefaultSolrParams in my code because I didn't check for non-existent defaults. I noticed through the Solr codebase that is code pattern is very common:
> {code:java}
> if( defaults != null ) {
> params = new DefaultSolrParams( params, defaults );
> }
> {code}
> Instead, I refactored this logic into a new SolrParams.combine(p,d) method and made it so that nobody refers to DefaultSolrParams. I did similarly for AppendedSolrParams.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[jira] [Commented] (SOLR-2920) Refactor frequent conditional use of
DefaultSolrParams into SolrParams.combine(p,d)
Posted by "Hoss Man (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-2920?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13159522#comment-13159522 ]
Hoss Man commented on SOLR-2920:
--------------------------------
David:
+1 to the refactoring ... thta's a great idea to help eliminate bugs
-0 to the names SolrParams.combine and SolrParams.combineAppended .. "combine" doesn't really make it clear what it's doing, it sounds more like a merge then wrapping in a defaults.
What do you think about putting each of these static methods in their respective classes?... "params = DefaultSolrParams.wrap(params,defaults)" and "params = AppendedSolrParamswrap(params,extra)" ?
(BTW: can you config your IDE to not reorder import statements? makes patches a lot harder to review)
> Refactor frequent conditional use of DefaultSolrParams into SolrParams.combine(p,d)
> -----------------------------------------------------------------------------------
>
> Key: SOLR-2920
> URL: https://issues.apache.org/jira/browse/SOLR-2920
> Project: Solr
> Issue Type: Improvement
> Reporter: David Smiley
> Priority: Minor
> Attachments: SOLR-2920_SolrParams_combine().patch
>
>
> I had a small bug in use of DefaultSolrParams in my code because I didn't check for non-existent defaults. I noticed through the Solr codebase that is code pattern is very common:
> {code:java}
> if( defaults != null ) {
> params = new DefaultSolrParams( params, defaults );
> }
> {code}
> Instead, I refactored this logic into a new SolrParams.combine(p,d) method and made it so that nobody refers to DefaultSolrParams. I did similarly for AppendedSolrParams.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[jira] [Commented] (SOLR-2920) Refactor frequent conditional use of
DefaultSolrParams into SolrParams.combine(p,d)
Posted by "Hoss Man (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-2920?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13165442#comment-13165442 ]
Hoss Man commented on SOLR-2920:
--------------------------------
bq. I started this by putting a method in the respective class it came from, but then I noticed that SolrParams already has some handy static utility methods. Then it occurred to me that it would be convenient if the caller didn't need to know about all the classes in this package
Ah ... good point. SolrParams already has static methods like this, and no reason to increase the import count.
SolrParams.wrapAppended and SolrParams.wrapDefaults are a good call.
> Refactor frequent conditional use of DefaultSolrParams into SolrParams.combine(p,d)
> -----------------------------------------------------------------------------------
>
> Key: SOLR-2920
> URL: https://issues.apache.org/jira/browse/SOLR-2920
> Project: Solr
> Issue Type: Improvement
> Reporter: David Smiley
> Priority: Minor
> Attachments: SOLR-2920_SolrParams_combine().patch
>
>
> I had a small bug in use of DefaultSolrParams in my code because I didn't check for non-existent defaults. I noticed through the Solr codebase that is code pattern is very common:
> {code:java}
> if( defaults != null ) {
> params = new DefaultSolrParams( params, defaults );
> }
> {code}
> Instead, I refactored this logic into a new SolrParams.combine(p,d) method and made it so that nobody refers to DefaultSolrParams. I did similarly for AppendedSolrParams.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[jira] [Updated] (SOLR-2920) Refactor frequent conditional use of
DefaultSolrParams into SolrParams.combine(p,d)
Posted by "David Smiley (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-2920?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
David Smiley updated SOLR-2920:
-------------------------------
Attachment: SOLR-2920_SolrParams_wrapDefaults_and_wrapAppended.patch
I attached a new patch using wrapDefaults and wrapAppended, and I took care to retain the import statements according to Lucene/Solr's strict standards ;-)
I think it's still an open-question if we want to more aggressively take the SolrParams factory-method approach and create some methods that return a MapSolrParams, ModifiableSolrParams, MultiMapSolrParams, and RequiredSolrParams. Personally, I really like the idea of SolrParams being a one-stop shop for these.
> Refactor frequent conditional use of DefaultSolrParams into SolrParams.combine(p,d)
> -----------------------------------------------------------------------------------
>
> Key: SOLR-2920
> URL: https://issues.apache.org/jira/browse/SOLR-2920
> Project: Solr
> Issue Type: Improvement
> Reporter: David Smiley
> Priority: Minor
> Attachments: SOLR-2920_SolrParams_combine().patch, SOLR-2920_SolrParams_wrapDefaults_and_wrapAppended.patch
>
>
> I had a small bug in use of DefaultSolrParams in my code because I didn't check for non-existent defaults. I noticed through the Solr codebase that is code pattern is very common:
> {code:java}
> if( defaults != null ) {
> params = new DefaultSolrParams( params, defaults );
> }
> {code}
> Instead, I refactored this logic into a new SolrParams.combine(p,d) method and made it so that nobody refers to DefaultSolrParams. I did similarly for AppendedSolrParams.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org