You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/07/03 18:47:39 UTC
[GitHub] [solr] epugh opened a new pull request, #929: SOLR-16280: simplify for loop
epugh opened a new pull request, #929:
URL: https://github.com/apache/solr/pull/929
https://issues.apache.org/jira/browse/SOLR-16280
# Description
Simplify for loop use
# Solution
Please provide a short description of the approach taken to implement your solution.
# Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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: issues-unsubscribe@solr.apache.org
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
[GitHub] [solr] cpoerschke commented on a diff in pull request #929: SOLR-16280: simplify for loop
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on code in PR #929:
URL: https://github.com/apache/solr/pull/929#discussion_r914001900
##########
solr/core/src/test/org/apache/solr/schema/TestPointFields.java:
##########
@@ -5289,9 +5289,9 @@ private void doTestSetQueries(String fieldName, String[] values, boolean multiVa
SchemaField sf = h.getCore().getLatestSchema().getField(fieldName);
assertTrue(sf.getType() instanceof PointField);
- for (int i = 0; i < values.length; i++) {
+ for (String s : values) {
Review Comment:
minor: here `s` is chosen but below `value` is chosen. consistency can help with code reading e.g. `value` in both places or `s` or `v` or so.
##########
solr/core/src/test/org/apache/solr/cloud/TestStressInPlaceUpdates.java:
##########
@@ -684,10 +684,9 @@ public SolrClient getClientForLeader() throws KeeperException, InterruptedExcept
Slice shard1 = clusterState.getCollection(DEFAULT_COLLECTION).getSlice(SHARD1);
leader = shard1.getLeader();
- for (int i = 0; i < clients.size(); i++) {
+ for (SolrClient client : clients) {
String leaderBaseUrl = zkStateReader.getBaseUrlForNodeName(leader.getNodeName());
- if (((HttpSolrClient) clients.get(i)).getBaseURL().startsWith(leaderBaseUrl))
- return clients.get(i);
+ if (((HttpSolrClient) client).getBaseURL().startsWith(leaderBaseUrl)) return client;
Review Comment:
```suggestion
if (((HttpSolrClient) client).getBaseURL().startsWith(leaderBaseUrl)) {
return client;
}
```
##########
solr/core/src/test/org/apache/solr/search/QueryEqualityTest.java:
##########
@@ -1351,13 +1351,13 @@ protected void assertQueryEquals(
SolrRequestInfo.clearRequestInfo();
}
- for (int i = 0; i < queries.length; i++) {
- QueryUtils.check(queries[i]);
+ for (Query value : queries) {
+ QueryUtils.check(value);
// yes starting j=0 is redundent, we're making sure every query
// is equal to itself, and that the quality checks work regardless
// of which caller/callee is used.
- for (int j = 0; j < queries.length; j++) {
- QueryUtils.checkEqual(queries[i], queries[j]);
+ for (Query query : queries) {
+ QueryUtils.checkEqual(value, query);
Review Comment:
How about naming to indicate that both are from `queries` ultimately? E.g.
```suggestion
QueryUtils.checkEqual(query1, query2);
```
##########
solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesDistrib.java:
##########
@@ -220,9 +220,8 @@ private void mapReplicasToClients() throws KeeperException, InterruptedException
leader = shard1.getLeader();
String leaderBaseUrl = zkStateReader.getBaseUrlForNodeName(leader.getNodeName());
- for (int i = 0; i < clients.size(); i++) {
- if (((HttpSolrClient) clients.get(i)).getBaseURL().startsWith(leaderBaseUrl))
- LEADER = clients.get(i);
+ for (SolrClient solrClient : clients) {
+ if (((HttpSolrClient) solrClient).getBaseURL().startsWith(leaderBaseUrl)) LEADER = solrClient;
Review Comment:
```suggestion
if (((HttpSolrClient) solrClient).getBaseURL().startsWith(leaderBaseUrl)) {
LEADER = solrClient;
}
```
##########
solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesDistrib.java:
##########
@@ -231,9 +230,8 @@ private void mapReplicasToClients() throws KeeperException, InterruptedException
continue;
}
String baseUrl = zkStateReader.getBaseUrlForNodeName(rep.getNodeName());
- for (int i = 0; i < clients.size(); i++) {
- if (((HttpSolrClient) clients.get(i)).getBaseURL().startsWith(baseUrl))
- NONLEADERS.add(clients.get(i));
+ for (SolrClient client : clients) {
+ if (((HttpSolrClient) client).getBaseURL().startsWith(baseUrl)) NONLEADERS.add(client);
Review Comment:
```suggestion
if (((HttpSolrClient) client).getBaseURL().startsWith(baseUrl)) {
NONLEADERS.add(client);
}
```
##########
solr/core/src/test/org/apache/solr/update/processor/SignatureUpdateProcessorFactoryTest.java:
##########
@@ -179,20 +179,20 @@ public void run() {
threads2[i].setName("testThread2-" + i);
}
- for (int i = 0; i < threads.length; i++) {
- threads[i].start();
+ for (Thread element : threads) {
+ element.start();
}
- for (int i = 0; i < threads2.length; i++) {
- threads2[i].start();
+ for (Thread item : threads2) {
+ item.start();
}
- for (int i = 0; i < threads.length; i++) {
- threads[i].join();
+ for (Thread value : threads) {
+ value.join();
}
Review Comment:
Curious about the `element/item/value` naming choices here, would `thread` be clearer?
--
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: issues-unsubscribe@solr.apache.org
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
[GitHub] [solr] epugh commented on a diff in pull request #929: SOLR-16280: simplify for-loops in tests
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #929:
URL: https://github.com/apache/solr/pull/929#discussion_r914938935
##########
solr/core/src/test/org/apache/solr/internal/csv/CSVParserTest.java:
##########
@@ -330,8 +330,8 @@ public void testEndOfFileBehaviorCSV() throws Exception {
{"world", ""}
};
String code;
- for (int codeIndex = 0; codeIndex < codes.length; codeIndex++) {
- code = codes[codeIndex];
+ for (String s : codes) {
Review Comment:
yep!
--
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: issues-unsubscribe@solr.apache.org
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
[GitHub] [solr] cpoerschke commented on a diff in pull request #929: SOLR-16280: simplify for-loops in tests
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on code in PR #929:
URL: https://github.com/apache/solr/pull/929#discussion_r915025303
##########
solr/core/src/test/org/apache/solr/internal/csv/CSVParserTest.java:
##########
@@ -352,8 +349,8 @@ public void testEmptyLineBehaviourExcel() throws Exception {
{""}
};
String code;
Review Comment:
```suggestion
```
##########
solr/core/src/test/org/apache/solr/internal/csv/CSVParserTest.java:
##########
@@ -352,8 +349,8 @@ public void testEmptyLineBehaviourExcel() throws Exception {
{""}
};
String code;
- for (int codeIndex = 0; codeIndex < codes.length; codeIndex++) {
- code = codes[codeIndex];
+ for (String s : codes) {
+ code = s;
Review Comment:
```suggestion
for (String code : codes) {
```
##########
solr/core/src/test/org/apache/solr/internal/csv/CSVParserTest.java:
##########
@@ -372,8 +369,8 @@ public void testEmptyLineBehaviourCSV() throws Exception {
{"hello", ""} // CSV Strategy ignores empty lines
};
String code;
Review Comment:
```suggestion
```
##########
solr/core/src/test/org/apache/solr/internal/csv/CSVParserTest.java:
##########
@@ -227,10 +227,10 @@ public void testGetLine() throws IOException {
public void testNextValue() throws IOException {
CSVParser parser = new CSVParser(new StringReader(code));
String tmp = null;
- for (int i = 0; i < res.length; i++) {
- for (int j = 0; j < res[i].length; j++) {
+ for (String[] re : res) {
+ for (int j = 0; j < re.length; j++) {
Review Comment:
```suggestion
for (String r : re) {
```
##########
solr/core/src/test/org/apache/solr/internal/csv/CSVParserTest.java:
##########
@@ -372,8 +369,8 @@ public void testEmptyLineBehaviourCSV() throws Exception {
{"hello", ""} // CSV Strategy ignores empty lines
};
String code;
- for (int codeIndex = 0; codeIndex < codes.length; codeIndex++) {
- code = codes[codeIndex];
+ for (String s : codes) {
+ code = s;
Review Comment:
```suggestion
for (String code : codes) {
```
--
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: issues-unsubscribe@solr.apache.org
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
[GitHub] [solr] epugh commented on pull request #929: SOLR-16280: simplify for-loops in tests
Posted by GitBox <gi...@apache.org>.
epugh commented on PR #929:
URL: https://github.com/apache/solr/pull/929#issuecomment-1176164427
@cpoerschke based on the changes, do you think this is ready?
--
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: issues-unsubscribe@solr.apache.org
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
[GitHub] [solr] epugh commented on a diff in pull request #929: SOLR-16280: simplify for-loops in tests
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #929:
URL: https://github.com/apache/solr/pull/929#discussion_r914776978
##########
solr/core/src/test/org/apache/solr/update/processor/SignatureUpdateProcessorFactoryTest.java:
##########
@@ -179,20 +179,20 @@ public void run() {
threads2[i].setName("testThread2-" + i);
}
- for (int i = 0; i < threads.length; i++) {
- threads[i].start();
+ for (Thread element : threads) {
+ element.start();
}
- for (int i = 0; i < threads2.length; i++) {
- threads2[i].start();
+ for (Thread item : threads2) {
+ item.start();
}
- for (int i = 0; i < threads.length; i++) {
- threads[i].join();
+ for (Thread value : threads) {
+ value.join();
}
Review Comment:
Made a change, let me know what you think?
--
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: issues-unsubscribe@solr.apache.org
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
[GitHub] [solr] cpoerschke commented on a diff in pull request #929: SOLR-16280: simplify for-loops in tests
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on code in PR #929:
URL: https://github.com/apache/solr/pull/929#discussion_r914909077
##########
solr/core/src/test/org/apache/solr/internal/csv/CSVParserTest.java:
##########
@@ -330,8 +330,8 @@ public void testEndOfFileBehaviorCSV() throws Exception {
{"world", ""}
};
String code;
- for (int codeIndex = 0; codeIndex < codes.length; codeIndex++) {
- code = codes[codeIndex];
+ for (String s : codes) {
Review Comment:
as above w.r.t. eliminating the `code` variable before the loop here, also similar changes in below
##########
solr/core/src/test/org/apache/solr/internal/csv/CSVParserTest.java:
##########
@@ -302,8 +302,8 @@ public void testEndOfFileBehaviourExcel() throws Exception {
{"world", ""}
};
String code;
- for (int codeIndex = 0; codeIndex < codes.length; codeIndex++) {
- code = codes[codeIndex];
+ for (String s : codes) {
+ code = s;
Review Comment:
```suggestion
for (String code : codes) {
```
##########
solr/core/src/test/org/apache/solr/internal/csv/CSVParserTest.java:
##########
@@ -579,10 +579,10 @@ public void testLineTokenConsistency() throws IOException {
String[][] data = parser.getAllValues();
parser = new CSVParser(new StringReader(code));
CSVParser parser1 = new CSVParser(new StringReader(code));
- for (int i = 0; i < data.length; i++) {
- assertTrue(Arrays.equals(parser1.getLine(), data[i]));
- for (int j = 0; j < data[i].length; j++) {
- assertEquals(parser.nextValue(), data[i][j]);
+ for (String[] datum : data) {
+ assertTrue(Arrays.equals(parser1.getLine(), datum));
+ for (int j = 0; j < datum.length; j++) {
+ assertEquals(parser.nextValue(), datum[j]);
Review Comment:
```suggestion
for (String d : datum) {
assertEquals(parser.nextValue(), d);
```
##########
solr/core/src/test/org/apache/solr/internal/csv/CSVParserTest.java:
##########
@@ -227,10 +227,10 @@ public void testGetLine() throws IOException {
public void testNextValue() throws IOException {
CSVParser parser = new CSVParser(new StringReader(code));
String tmp = null;
- for (int i = 0; i < res.length; i++) {
- for (int j = 0; j < res[i].length; j++) {
+ for (String[] re : res) {
+ for (int j = 0; j < re.length; j++) {
tmp = parser.nextValue();
- assertEquals(res[i][j], tmp);
+ assertEquals(re[j], tmp);
Review Comment:
```suggestion
assertEquals(r, tmp);
```
##########
solr/core/src/test/org/apache/solr/internal/csv/CSVParserTest.java:
##########
@@ -227,10 +227,10 @@ public void testGetLine() throws IOException {
public void testNextValue() throws IOException {
CSVParser parser = new CSVParser(new StringReader(code));
String tmp = null;
- for (int i = 0; i < res.length; i++) {
- for (int j = 0; j < res[i].length; j++) {
+ for (String[] re : res) {
+ for (int j = 0; j < re.length; j++) {
Review Comment:
```suggestion
for (String r : re) {
```
##########
solr/core/src/test/org/apache/solr/internal/csv/CSVParserTest.java:
##########
@@ -302,8 +302,8 @@ public void testEndOfFileBehaviourExcel() throws Exception {
{"world", ""}
};
String code;
Review Comment:
```suggestion
```
--
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: issues-unsubscribe@solr.apache.org
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
[GitHub] [solr] epugh merged pull request #929: SOLR-16280: simplify for-loops in tests
Posted by GitBox <gi...@apache.org>.
epugh merged PR #929:
URL: https://github.com/apache/solr/pull/929
--
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: issues-unsubscribe@solr.apache.org
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
[GitHub] [solr] cpoerschke commented on a diff in pull request #929: SOLR-16280: simplify for-loops in tests
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on code in PR #929:
URL: https://github.com/apache/solr/pull/929#discussion_r915176340
##########
solr/core/src/test/org/apache/solr/internal/csv/CSVParserTest.java:
##########
@@ -351,9 +348,8 @@ public void testEmptyLineBehaviourExcel() throws Exception {
{""}, // ExcelStrategy does not ignore empty lines
{""}
};
- String code;
- for (int codeIndex = 0; codeIndex < codes.length; codeIndex++) {
- code = codes[codeIndex];
+ for (String s : codes) {
+ code = s;
Review Comment:
```suggestion
for (String code : codes) {
```
--
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: issues-unsubscribe@solr.apache.org
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