[GitHub] drill pull request #1235: DRILL-6336: Inconsistent method name.

[BruceKuiLiu <gi...@git.apache.org>]

GitHub user BruceKuiLiu opened a pull request:

    https://github.com/apache/drill/pull/1235

    DRILL-6336: Inconsistent method name.

    Change the method name "append" to "print" since its body code is an method invocation "writer.print( s )". The method named "print" should be more clear.
    The method name "append" is labeled as "@Deprecated" in case of breaking backward compatibility.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/BruceKuiLiu/drill master

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/drill/pull/1235.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 #1235
    
----
commit d501d338f23acd60164d85c36f5143426a426ea3
Author: Kui LIU <br...@...>
Date:   2018-04-17T15:45:44Z

    Rename method.

----


---

[GitHub] drill issue #1235: DRILL-6336: Inconsistent method name.

[arina-ielchiieva <gi...@git.apache.org>]

Github user arina-ielchiieva commented on the issue:

    https://github.com/apache/drill/pull/1235
  
    @BruceKuiLiu it is not public interface, no need to deprecate the method. You can simply replace.


---

[GitHub] drill issue #1235: DRILL-6336: Inconsistent method name.

[vrozov <gi...@git.apache.org>]

Github user vrozov commented on the issue:

    https://github.com/apache/drill/pull/1235
  
    My take is that "append" is more common for classes with the similar functionality, see for example `ToStringBuilder`. As there is no added benefit of using "print" vs "append", my recommendation is to keep "append" as is and see if `DebugStringBuilder` can be replaced with the `ToStringBuilder`.


---

[GitHub] drill issue #1235: DRILL-6336: Inconsistent method name.

[vrozov <gi...@git.apache.org>]

Github user vrozov commented on the issue:

    https://github.com/apache/drill/pull/1235
  
    There is no need to expose implementation details as part of the class API. Whether `DebugStringBuilder` uses `PrintWriter.print()` or something else to implement `append()` must be hidden from `DebugStringBuilder` consumers. Method name should never depend on details of implementation as the implementation may change, but API should not.


---

[GitHub] drill issue #1235: DRILL-6336: Inconsistent method name.

[arina-ielchiieva <gi...@git.apache.org>]

Github user arina-ielchiieva commented on the issue:

    https://github.com/apache/drill/pull/1235
  
    @vrozov so you suggest to leave as is, correct?
    @paul-rogers since you have originally added `DebugStringBuilder`, do you agree?


---

[GitHub] drill issue #1235: DRILL-6336: Inconsistent method name.

[paul-rogers <gi...@git.apache.org>]

Github user paul-rogers commented on the issue:

    https://github.com/apache/drill/pull/1235
  
    @arina-ielchiieva, the `DebugStringBuilder` class probably started as a wrapper around a `StringBuilder` at some point, given its name. Looks like it is now a wrapper around a `StringWriter`. Still, the goal of this class is to take a bunch of arguments and produce a string. That string can be printed, sent to a log, or just displayed in the debugger. (It is the debugger case that I was probably concerned about; some classes threw exceptions if you tried to examine them in the debugger.)
    
    Given that this class is a string *builder* and not a string *printer*, the `append()` method evokes the `append()` method in `StringBuilder`. This may be the point that @vrozov is making.
    
    That said, there is certainly no harm in changing the name if it helps people understand the purpose of the class more clearly. Since this is purely an internal debug class, either name is fine.


---

[GitHub] drill issue #1235: DRILL-6336: Inconsistent method name.

[BruceKuiLiu <gi...@git.apache.org>]

Github user BruceKuiLiu commented on the issue:

    https://github.com/apache/drill/pull/1235
  
    Thanks.


---