Rename method `visit` of `ASTNode` to `accept`

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

Rename method `visit` of `ASTNode` to `accept`

Daniel.Sun
Hi all,

        According to the naming conventions of Visitor Pattern[1], I propose
to rename method `visit` of `ASTNode`[2] to `accept` .

        Any thoughts?

Cheers,
Daniel.Sun
[1] https://en.wikipedia.org/wiki/Visitor_pattern#Java_example
[2]
https://github.com/apache/groovy/blob/master/src/main/java/org/codehaus/groovy/ast/ASTNode.java#L53



-----
Apache Groovy committer
Blog: http://blog.sunlan.me 
Twitter: @daniel_sun

--
Sent from: http://groovy.329449.n5.nabble.com/Groovy-Dev-f372993.html
Apache Groovy committer & PMC member

Blog: http://blog.sunlan.me
Twitter: @daniel_sun
Reply | Threaded
Open this post in threaded view
|

Re: Rename method `visit` of `ASTNode` to `accept`

paulk_asert
I would be cautious here. Yes, moving to the more conventional name might
have some benefits but such a breaking change (even though within the
org.codehaus.groovy "internal" part of the codebase) might impact many
related projects and even end users. I would ask on the users list who uses
that method. We don't necessarily want to force IDEs and Spock etc. to
have to have two different versions to cater for this. An alias is a possibly
more friendly way to introduce such a change, but I am not sure I'd even
deprecate the old method straight away.

Cheers, Paul.


On Sun, Mar 24, 2019 at 2:33 AM Daniel.Sun <[hidden email]> wrote:
Hi all,

        According to the naming conventions of Visitor Pattern[1], I propose
to rename method `visit` of `ASTNode`[2] to `accept` .

        Any thoughts?

Cheers,
Daniel.Sun
[1] https://en.wikipedia.org/wiki/Visitor_pattern#Java_example
[2]
https://github.com/apache/groovy/blob/master/src/main/java/org/codehaus/groovy/ast/ASTNode.java#L53



-----
Apache Groovy committer
Blog: http://blog.sunlan.me
Twitter: @daniel_sun

--
Sent from: http://groovy.329449.n5.nabble.com/Groovy-Dev-f372993.html
Reply | Threaded
Open this post in threaded view
|

Re: Rename method `visit` of `ASTNode` to `accept`

Andrew Bayer
We use it extensively in Jenkins Pipeline and script security.

A. 

On Sat, Mar 23, 2019, 16:10 Paul King <[hidden email]> wrote:
I would be cautious here. Yes, moving to the more conventional name might
have some benefits but such a breaking change (even though within the
org.codehaus.groovy "internal" part of the codebase) might impact many
related projects and even end users. I would ask on the users list who uses
that method. We don't necessarily want to force IDEs and Spock etc. to
have to have two different versions to cater for this. An alias is a possibly
more friendly way to introduce such a change, but I am not sure I'd even
deprecate the old method straight away.

Cheers, Paul.


On Sun, Mar 24, 2019 at 2:33 AM Daniel.Sun <[hidden email]> wrote:
Hi all,

        According to the naming conventions of Visitor Pattern[1], I propose
to rename method `visit` of `ASTNode`[2] to `accept` .

        Any thoughts?

Cheers,
Daniel.Sun
[1] https://en.wikipedia.org/wiki/Visitor_pattern#Java_example
[2]
https://github.com/apache/groovy/blob/master/src/main/java/org/codehaus/groovy/ast/ASTNode.java#L53



-----
Apache Groovy committer
Blog: http://blog.sunlan.me
Twitter: @daniel_sun

--
Sent from: http://groovy.329449.n5.nabble.com/Groovy-Dev-f372993.html
Reply | Threaded
Open this post in threaded view
|

Re: Rename method `visit` of `ASTNode` to `accept`

Daniel.Sun
In reply to this post by paulk_asert
OK. Let's add an alias method `accept` and mark method `visit` deprecated.

Cheers,
Daniel.Sun




-----
Apache Groovy committer
Blog: http://blog.sunlan.me 
Twitter: @daniel_sun

--
Sent from: http://groovy.329449.n5.nabble.com/Groovy-Dev-f372993.html
Apache Groovy committer & PMC member

Blog: http://blog.sunlan.me
Twitter: @daniel_sun
Reply | Threaded
Open this post in threaded view
|

Re: Rename method `visit` of `ASTNode` to `accept`

paulk_asert
If you want to deprecate now and not in the future, I think that would still warrant a question on the users list.
People often feel compelled to get rid of the deprecation warnings and that still means two versions of their libraries would be required.
I'd be inclined to wait until Groovy 4 for deprecation.


On Sun, Mar 24, 2019 at 11:05 AM Daniel.Sun <[hidden email]> wrote:
OK. Let's add an alias method `accept` and mark method `visit` deprecated.

Cheers,
Daniel.Sun




-----
Apache Groovy committer
Blog: http://blog.sunlan.me
Twitter: @daniel_sun

--
Sent from: http://groovy.329449.n5.nabble.com/Groovy-Dev-f372993.html
Reply | Threaded
Open this post in threaded view
|

Re: Rename method `visit` of `ASTNode` to `accept`

paulk_asert
Once we have Java 9 as our minimum, we can use the `since` and `forRemoval` Deprecated attributes
and that will allow us to make such deprecations a little clearer. 

On Sun, Mar 24, 2019 at 12:26 PM Paul King <[hidden email]> wrote:
If you want to deprecate now and not in the future, I think that would still warrant a question on the users list.
People often feel compelled to get rid of the deprecation warnings and that still means two versions of their libraries would be required.
I'd be inclined to wait until Groovy 4 for deprecation.


On Sun, Mar 24, 2019 at 11:05 AM Daniel.Sun <[hidden email]> wrote:
OK. Let's add an alias method `accept` and mark method `visit` deprecated.

Cheers,
Daniel.Sun




-----
Apache Groovy committer
Blog: http://blog.sunlan.me
Twitter: @daniel_sun

--
Sent from: http://groovy.329449.n5.nabble.com/Groovy-Dev-f372993.html
Reply | Threaded
Open this post in threaded view
|

Re: Rename method `visit` of `ASTNode` to `accept`

Guillaume Laforge
Administrator
What's the point of renaming that method?

Is it just to use a more well known / common name from the visitor pattern?
IMHO it's really not worth the hassle, and it's more pain for users who will ultimately have to recompile their code at some point.
It doesn't bring anything to the users in any way. Or am I missing something?

On Sun, Mar 24, 2019 at 3:31 AM Paul King <[hidden email]> wrote:
Once we have Java 9 as our minimum, we can use the `since` and `forRemoval` Deprecated attributes
and that will allow us to make such deprecations a little clearer. 

On Sun, Mar 24, 2019 at 12:26 PM Paul King <[hidden email]> wrote:
If you want to deprecate now and not in the future, I think that would still warrant a question on the users list.
People often feel compelled to get rid of the deprecation warnings and that still means two versions of their libraries would be required.
I'd be inclined to wait until Groovy 4 for deprecation.


On Sun, Mar 24, 2019 at 11:05 AM Daniel.Sun <[hidden email]> wrote:
OK. Let's add an alias method `accept` and mark method `visit` deprecated.

Cheers,
Daniel.Sun




-----
Apache Groovy committer
Blog: http://blog.sunlan.me
Twitter: @daniel_sun

--
Sent from: http://groovy.329449.n5.nabble.com/Groovy-Dev-f372993.html


--
Guillaume Laforge
Apache Groovy committer
Developer Advocate @ Google Cloud Platform

Reply | Threaded
Open this post in threaded view
|

Re: Rename method `visit` of `ASTNode` to `accept`

Daniel.Sun
Hi Guillaume,

     According to the concept of OO, node accepts visitor's visiting, so
`accept` is a correct name IMO.

     > What's the point of renaming that method?
     If we all agree with the above point, we should do the right thing,
right?  ;-)

Cheers,
Daniel.Sun



-----
Apache Groovy committer
Blog: http://blog.sunlan.me 
Twitter: @daniel_sun

--
Sent from: http://groovy.329449.n5.nabble.com/Groovy-Dev-f372993.html
Apache Groovy committer & PMC member

Blog: http://blog.sunlan.me
Twitter: @daniel_sun
Reply | Threaded
Open this post in threaded view
|

Re: Rename method `visit` of `ASTNode` to `accept`

Guillaume Laforge
Administrator
To me, it's just a cosmetic change that doesn't add any value, and has potentially bad impact on users.
So it's more harm than good. We should avoid such changes.

On Sun, Mar 24, 2019 at 11:46 AM Daniel.Sun <[hidden email]> wrote:
Hi Guillaume,

     According to the concept of OO, node accepts visitor's visiting, so
`accept` is a correct name IMO.

     > What's the point of renaming that method?
     If we all agree with the above point, we should do the right thing,
right?  ;-)

Cheers,
Daniel.Sun



-----
Apache Groovy committer
Blog: http://blog.sunlan.me
Twitter: @daniel_sun

--
Sent from: http://groovy.329449.n5.nabble.com/Groovy-Dev-f372993.html


--
Guillaume Laforge
Apache Groovy committer
Developer Advocate @ Google Cloud Platform

Reply | Threaded
Open this post in threaded view
|

Re: Rename method `visit` of `ASTNode` to `accept`

Daniel.Sun
Hi  Guillaume,

> To me, it's just a cosmetic change that doesn't add any value, and has
> potentially bad impact on users.
> So it's more harm than good. We should avoid such changes.
      Understood.

      Renaming method `visit` of `ASTNode` to `accept` and adding an alias
method `visit` should be able to keep the back compatibility. (See
https://github.com/apache/groovy/pull/902/files#diff-354afdf77aa674ebd8d201981cb5b723)

      In order to not disturb groovy users, we even do not add `@Deprecated`
to the alias method `visit`.

      To sum up, no worries :-)

Cheers,
Daniel.Sun



-----
Apache Groovy committer
Blog: http://blog.sunlan.me 
Twitter: @daniel_sun

--
Sent from: http://groovy.329449.n5.nabble.com/Groovy-Dev-f372993.html
Apache Groovy committer & PMC member

Blog: http://blog.sunlan.me
Twitter: @daniel_sun