Remove duplicate code of Landis.TextParser from extensions and libraries

GoogleCodeExporter commented 9 years ago

Issue 13 describes the related problem where the Landis.TextParser class is in 
the core's implementation instead of its API.  Consequently, the extensions and 
libraries have their parameter parsers derived from Flel.Util's TextParser 
instead of Landis.TextParser.  And therefore, they have duplicated code that's 
in Landis.TextParser.  Once issue 13 is fixed, these parsers need to have the 
duplicate code removed.

Original issue reported on code.google.com by jdomi...@green-code.com on 15 Jul 2012 at 9:38

Blocked on: #13

GoogleCodeExporter commented 9 years ago

Original comment by jdomi...@green-code.com on 15 Jul 2012 at 9:38

Now blocked on: #13

GoogleCodeExporter commented 9 years ago

These are the parsers that need fix:

         Century-succession  --> utility/InputParameterParser.cs
         age-only-succession --> DynamicInputParser.cs
                                 InputParametersParser.cs

                base-harvest --> InputParametersParser.cs
             biomass-harvest --> InputParametersParser.cs
             biomass-insects --> InputParameterParser.cs

          biomass-succession --> age-only-disturbances/DatasetParser.cs
                                 utility/DynamicInputParser.cs
                                 utility/InputParameterParser.cs

    clmate-generator-library --> ClimateParser.cs

  drought/
         drought-disturbance --> InputParameterParser.cs
           drought-generator --> InputParameterParser.cs

       dynamic-biomass-fuels --> InputParametersParser.cs
                dynamic-fire --> InputParameterParser.cs
               dynamic-fuels --> InputParametersParser.cs
  dynamic-leaf-biomass-fuels --> InputParametersParser.cs

        leaf-biomass-insects --> InputParameterParser.cs

       output-biomass-by-age --> InputParametersParser.cs
      output-biomass-reclass --> InputParametersParser.cs
              output-biomass --> InputParametersParser.cs
         output-cohort-stats --> InputParametersParser.cs
         output-leaf-biomass --> InputParametersParser.cs
      output-max-species-age --> InputParametersParser.cs

          succession-library --> initial-communities/DatasetParser.cs

Original comment by jdomi...@green-code.com on 15 Jul 2012 at 9:39

GoogleCodeExporter commented 9 years ago

The duplicate code looks something like:

    class FooParser
        : TextParser<IFooParameters>
    {
    ...

        protected override IFooParameters Parse()
        {
            InputVar<string> landisData = new InputVar<string>("LandisData");
            ReadVar(landisData);
            if (landisData.Value.Actual != PlugIn.ExtensionName)
                throw new InputValueException(landisData.Value.String, "The value is not \"{0}\"", PlugIn.ExtensionName);

            ... parse the parameters ...

==============================================================================

What the code SHOULD look like is:

    class FooParser
        : Landis.TextParser<IFooParameters>
    {
        public override string LandisDataValue
        {
            get {
                return "FooBarParameters";  // expected value for LandisData variable
            }
        }

        protected override IFooParameters Parse()
        {
            ReadLandisDataVar();

            ... parse the parameters ...

Original comment by jdomi...@green-code.com on 15 Jul 2012 at 9:40

GoogleCodeExporter commented 9 years ago

Discovered the origins of this issue.  On 5 August 2010 (in
revision 2815 of the Wisconsin repository), Sri separated the
core into two assemblies as directed.  One assembly
(Landis.Core) for public data types (core's API) and the
other for its private types (Landis.Core.Implementation).

  http://flel.landis-ii.org/websvn/revision.php?repname=LANDIS&path=%2Fcore%2F&rev=2815

However, the util module's Landis.TextParser was put into
the Implementation assembly, but it should've been put into
the API.  I should've realized this misplacement then
because all the input file parsers in LANDIS-II (core,
extensions, libraries, tools) derive from Landis.TextParser.

Consequently, when Sri updated various components on August
10, 2010 for the core separation, he incorrectly added the
core's implementation as a project reference:

  Output-reclass Extension, revision 2847:

    http://flel.landis-ii.org/websvn/diff.php?repname=LANDIS&path=%2Fplug-ins%2Freclass%2Fbranches%2F6.0-core%2Fsrc%2Foutput-reclass%2Foutput-reclass.csproj&rev=2847

  Succession library, revision 2849:

    http://flel.landis-ii.org/websvn/diff.php?repname=LANDIS&path=%2Flibs%2Fsuccession%2Fbranches%2F6.0-core%2Fsrc%2Fsuccession.csproj&rev=2849

In both revisions, the parser classes (the extension's
InputParametersParser and the library's DatasetParser for
initial communities) were still derived properly from the
Landis.TextParser base class.  Each derived class correctly
defined the LandisDataValue property, and called the
ReadLandisDataVar method in the base class.

A review of the diffs for these commits should have
identified this inappropriate reference to the core's
implementation.  These code reviews would have uncovered the
need to move Landis.TextParser to the core API (eventually
reported as issue #13).

Because this problem wasn't discovered then, when Rob
switched the NUC-succession extension over to C# IDE files
the next day (August 11):

  NuCSS succession, revision 2851:

    http://flel.landis-ii.org/websvn/revision.php?repname=LANDIS&path=%2F&rev=2851

the code couldn't compile because the C# project file didn't
include a reference to the core implementation.  In attempt
to fix this the following day (August 12), Rob added a
temporary hack to the ExtensionMain class in the core's API:

  ExtensionMain class, Core API, revision 2859:

    http://flel.landis-ii.org/websvn/diff.php?repname=LANDIS&path=%2Fcore%2Fbranches%2FLandisCore%2FLandis.Core%2FExtensionMain.cs&rev=2859

This solution was wrong, and Sri removed it four days later
(August 16, 2010):

  ExtensionMain class, Core API, revision 2866:

    http://flel.landis-ii.org/websvn/diff.php?repname=LANDIS&path=%2Fcore%2Fbranches%2FLandisCore%2FLandis.Core%2FExtensionMain.cs&rev=2866

In the very next revision (#2867, less than an hour later),
Sri modified the output-reclass extension to make it
"independent of Landis.Core.Implementation":

  http://flel.landis-ii.org/websvn/revision.php?repname=LANDIS&path=%2F&rev=2867

He correctly removed the reference to the core implementation:

  http://flel.landis-ii.org/websvn/diff.php?repname=LANDIS&path=%2Fplug-ins%2Freclass%2Fbranches%2F6.0-core%2Fsrc%2Foutput-reclass%2Foutput-reclass.csproj&rev=2867

But without this reference, Landis.TextParser is no longer
available.  So Sri incorrectly modified the
InputParameterParser class:

  http://flel.landis-ii.org/websvn/diff.php?repname=LANDIS&path=%2Fplug-ins%2Freclass%2Fbranches%2F6.0-core%2Fsrc%2FInputParametersParser.cs&rev=2867

Instead of deriving from Landis.TextParser, this class now
derived from the unqualified TextParser (which resolves to
the Flel Util's TextParser; which is the base class for
Landis.TextParser).  In essence, the class hierarchy was
altered.  Consequently, Sri had to copy the code from the
ReadLandisDataVar method in the Landis.TextParser class
into InputParametersParser.

That was a very strong signal to ask about access to the
Landis.TextParser class, and whether it should've been in
the core's API.  Instead, he continued to copy these
inappropriate modifications to 2 other components that same
day (August 16):

  Base Fire extension, revision 2868:

    http://flel.landis-ii.org/websvn/diff.php?repname=LANDIS&path=%2Fplug-ins%2Ffire%2Fbranches%2F6.0-core%2Fsrc%2FInputParameterParser.cs&rev=2868

  Succession library, revision 2872:

    http://flel.landis-ii.org/websvn/diff.php?repname=LANDIS&path=%2Flibs%2Fsuccession%2Fbranches%2F6.0-core%2Fsrc%2Finitial-communities%2FDatasetParser.cs&rev=2872

Sri passed this onto Rob, so they ended up spreading it to
the parser classes in 7 other extensions the next day
(August 17):

  Base Wind, revision 2877:

    http://flel.landis-ii.org/websvn/diff.php?repname=LANDIS&path=%2Fplug-ins%2Fwind%2Ftrunk%2Fsrc%2FInputParameterParser.cs&rev=2877

  Output Cohorts Stats, revision 2879:

    http://flel.landis-ii.org/websvn/revision.php?repname=LANDIS&path=%2F&rev=2879

  Output Biomass By Age, revision 2882:

    http://flel.landis-ii.org/websvn/diff.php?repname=LANDIS&path=%2Fplug-ins%2Foutput-biomass-by-age%2Ftrunk%2Fsrc%2FInputParametersParser.cs&rev=2882

  Output Biomass, revision 2890:

    http://flel.landis-ii.org/websvn/diff.php?repname=LANDIS&path=%2Fplug-ins%2Foutput-biomass%2Ftrunk%2Fsrc%2FInputParametersParser.cs&rev=2890

  Reclass extension, revision 2893:

    http://flel.landis-ii.org/websvn/diff.php?repname=LANDIS&path=%2Fplug-ins%2Freclass%2Ftrunk%2Fsrc%2FInputParametersParser.cs&rev=2893

  Base BDA extension, revision 2894:

    http://flel.landis-ii.org/websvn/diff.php?repname=LANDIS&path=%2Fplug-ins%2FBaseBDA%2Ftrunk%2Fsrc%2FInputParameterParser.cs&rev=2894

  Age-only succession, revision 2896:

    http://flel.landis-ii.org/websvn/diff.php?repname=LANDIS&path=%2Fplug-ins%2Fage-only-succession%2Ftrunk%2Fsrc%2FInputParametersParser.cs&rev=2896

Eventually, this propagated to all other components.  This
code replication with an unqualified TextParser name also
made all that component code fragile.  Fixing issue #13
breaks these components because of that unqualified name.

Original comment by jdomi...@green-code.com on 23 Oct 2012 at 9:08

Added labels: Priority-High
Removed labels: Priority-Medium

wallen003 / landis-extensions

Remove duplicate code of Landis.TextParser from extensions and libraries #14