[jira] [Updated] (GROOVY-8547) Factory pattern side-stepped by CompilerConfiguration/ParserVersion

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

[jira] [Updated] (GROOVY-8547) Factory pattern side-stepped by CompilerConfiguration/ParserVersion

JIRA jira@apache.org

     [ https://issues.apache.org/jira/browse/GROOVY-8547?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Eric Milles updated GROOVY-8547:
--------------------------------
    Description:
It seems to me the selection of the Antlr2 or Antlr4 parser could be implemented within a default implementation of {{ParserPluginFactory}} instead of introducing {{ParserVersion}} with separate get/set on {{CompilerConfiguration}}. I don't see the need for static factory methods {{antlr2()}} and {{antlr4()}} on {{ParserPluginFactory}}, their use from {{CompilerConfiguration}} and the strange class loading in {{ParserPluginFactory}}.

The factory pattern appears to me to be side-stepped in favor of a version constant. Is there still meaning in offering a config option of setting a {{ParserPluginFactory}}? Has this been used ever in the past?

Couldn't this suffice for a default in {{CompilerConfiguration}}? Then {{ParserVersion}} could be dropped.
{code:java}
    public ParserPluginFactory getPluginFactory() {
        if (pluginFactory == null) {
            pluginFactory = new ParserPluginFactory() {
                @Override
                public ParserPlugin createParserPlugin() {
                    return Boolean.getBoolean(GROOVY_ANTLR4_OPT) ? new Antlr4PluginFactory() : new AntlrParserPluginFactory();
                }
            }              
        }
        return pluginFactory;
    }
{code}

  was:
It seems to me the selection of the Antlr2 or Antlr4 parser could be implemented within a default implementation of {{ParserPluginFactory}} instead of introducing {{ParserVersion}} with separate get/set on {{CompilerConfiguration}}.  I don't see the need for static factory methods {{antlr2()}} and {{antlr4()}} on {{ParserPluginFactory}}, their use from {{CompilerConfiguration}} and the strange class loading in {{ParserPluginFactory}}.

The factory pattern appears to me to be side-stepped in favor of a version constant.  Is there still meaning is offering a config option of setting a {{ParserPluginFactory}}?  Has this been used ever in the past?

Couldn't this suffice for a default in {{CompilerConfiguration}}?  Then {{ParserVersion}} could be dropped.
{code:java}
    public ParserPluginFactory getPluginFactory() {
        if (pluginFactory == null) {
            pluginFactory = new ParserPluginFactory() {
                @Override
                public ParserPlugin createParserPlugin() {
                    return Boolean.getBoolean(GROOVY_ANTLR4_OPT) ? new Antlr4PluginFactory() : new AntlrParserPluginFactory();
                }
            }              
        }
        return pluginFactory;
    }
{code}


> Factory pattern side-stepped by CompilerConfiguration/ParserVersion
> -------------------------------------------------------------------
>
>                 Key: GROOVY-8547
>                 URL: https://issues.apache.org/jira/browse/GROOVY-8547
>             Project: Groovy
>          Issue Type: Improvement
>          Components: parser
>            Reporter: Eric Milles
>            Priority: Minor
>
> It seems to me the selection of the Antlr2 or Antlr4 parser could be implemented within a default implementation of {{ParserPluginFactory}} instead of introducing {{ParserVersion}} with separate get/set on {{CompilerConfiguration}}. I don't see the need for static factory methods {{antlr2()}} and {{antlr4()}} on {{ParserPluginFactory}}, their use from {{CompilerConfiguration}} and the strange class loading in {{ParserPluginFactory}}.
> The factory pattern appears to me to be side-stepped in favor of a version constant. Is there still meaning in offering a config option of setting a {{ParserPluginFactory}}? Has this been used ever in the past?
> Couldn't this suffice for a default in {{CompilerConfiguration}}? Then {{ParserVersion}} could be dropped.
> {code:java}
>     public ParserPluginFactory getPluginFactory() {
>         if (pluginFactory == null) {
>             pluginFactory = new ParserPluginFactory() {
>                 @Override
>                 public ParserPlugin createParserPlugin() {
>                     return Boolean.getBoolean(GROOVY_ANTLR4_OPT) ? new Antlr4PluginFactory() : new AntlrParserPluginFactory();
>                 }
>             }              
>         }
>         return pluginFactory;
>     }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)