You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by BruceKuiLiu <gi...@git.apache.org> on 2018/04/21 20:04:34 UTC
[GitHub] drill pull request #1236: DRILL-6347: Inconsistent method name "field".
GitHub user BruceKuiLiu opened a pull request:
https://github.com/apache/drill/pull/1236
DRILL-6347: Inconsistent method name "field".
The method is names as "field", but the method is mainly doing appending. So that, rename the method as "append" should be better.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/BruceKuiLiu/drill field
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/1236.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1236
----
commit ecef20386237eb154555ec76e8cf4eabac022c38
Author: Kui LIU <br...@...>
Date: 2018-04-21T20:03:56Z
Change the method name 'field' to 'append'.
----
---
[GitHub] drill issue #1236: DRILL-6347: Inconsistent method name "field".
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1236
@BruceKuiLiu could you please address @vrozov comments?
---
[GitHub] drill pull request #1236: DRILL-6347: Inconsistent method name "field".
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/1236#discussion_r183341109
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/PrelVisualizerVisitor.java ---
@@ -88,10 +88,10 @@ public void endFields() {
}
public void field(String label, boolean value) {
- field(label, Boolean.toString(value));
+ append(label, Boolean.toString(value));
}
- private void field(String label, String value) {
+ private void append(String label, String value) {
--- End diff --
`appendField`?
---
[GitHub] drill issue #1236: DRILL-6347: Inconsistent method name "field".
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/1236
LGTM. Please squash commits.
---
[GitHub] drill pull request #1236: DRILL-6347: Inconsistent method name "field".
Posted by HanumathRao <gi...@git.apache.org>.
Github user HanumathRao commented on a diff in the pull request:
https://github.com/apache/drill/pull/1236#discussion_r183384261
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/PrelVisualizerVisitor.java ---
@@ -88,10 +88,10 @@ public void endFields() {
}
public void field(String label, boolean value) {
- field(label, Boolean.toString(value));
+ append(label, Boolean.toString(value));
}
- private void field(String label, String value) {
+ private void append(String label, String value) {
--- End diff --
Thanks Bruce for the changes. As Arina pointed out, appendField would be more meaningful in this context.
---
[GitHub] drill issue #1236: DRILL-6347: Inconsistent method name "field".
Posted by BruceKuiLiu <gi...@git.apache.org>.
Github user BruceKuiLiu commented on the issue:
https://github.com/apache/drill/pull/1236
@vrozov Thanks.
---
[GitHub] drill issue #1236: DRILL-6347: Inconsistent method name "field".
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/1236
IMO, neither `append` or `appendField` is a good choice (otherwise it is necessary to change `startNode/endNode` to `appendStart/EndNode`). It is either `field` or `visitField` and should follow visitor design pattern. It may be also good to change `listField` to the same name used for `Boolean` and `String`.
---