sudeep87 / uimafit

Automatically exported from code.google.com/p/uimafit
0 stars 0 forks source link

rearrange util package #8

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
Philip:
The util package looks pretty incoherent right now.  I think we should
rearrange things a bit.  I'm not sure what the package names should be but
here are some groupings that probably should be in the same package (along
with a proposed package name:

org.uutuc.util.uima 
AnnotationRetrieval

org.uutuc.util.test
DisableLogging
HideOutput
TearDownUtil (I think this can actually be replaced by constructs in junit)

org.uutuc.component
JCasAnnotatorAdapter
SingleFileXReader

org.uutuc.pipeline
SimplePipeline
JCasIterable

I'm not sure where ReflectionUtil should go - maybe this code should just
be folded into the class(es) that uses it.

TokenFactory should be moved out of the factory package.  Even though it is
a factory
- it is really not like the other factories and should go somewhere else.

Richard:
We have a number of almost-drop-in replacements for the UIMA component base
classes that I would like to 
contribute to the org.uutuc.component package:

CasAnnotator_ImplBase
CasConsumer_ImplBase  (actually a single-deploy CasAnnotator)
CasCollectionReader_ImplBase
JCasAnnotator_ImplBase
JCasCollectionReader_ImplBase
JCasConsumer_ImplBase (actually a single-deploy JCasAnnotator)

Almost-drop-in because the do not follow the confusing UIMA implementation
of having different initialize() 
signatures for every type of component, but rather providing the same
signature for all (initialize(UIMAContext)).

I would call the org.uutuc.util.test package org.uutuc.testing.util
instead, since the "test" aspect is dominant here, 
not the util aspect. Said differently, there is code (mainly) used when
testing and code used at runtime and I think it 
makes sense in grouping code belonging to these two scopes together. Then
the TokenFactory should probably go 
to org.uutuc.testing.component or org.uutuc.testing.factory. 

Richard:
Ah, what I forgot to say. These base-classes of course invoke the init
utils to inject the UIMA context parameters 
and shared resources into class fields. For this to work properly when
initialize(UIMAContext) is overwritten, 
always needs to call super.initialize(context). I am thinking though about
introducing a no-args initialize 
method (maybe called init instead) that will not require subclasses to
explicitly invoke super.

Philip:
These base classes sound really nice and would be something I would
personally want
to use.  Do they inherit from the e.g. uima JCasAnnotator_ImplBase?  

I am not clear why requiring subclasses to call super.initialize is a
problem.  It
seems reasonable to require it as part of the contract of overriding the
initialize
method.  

Regardless, it sounds promising!

Richard:
I'd prefer adding them to the repository and then discuss if there are
issues instead of going through that routine 
of posting a patch etc. Is that ok with you?

Philip:
certainly!  I'm not sure why/when the patch process started.  I think in
general, if
current behavior is going to change, then we should discuss how to go about
adding it
(patch or direct commit).  For new features, classes etc. I don't see any
reason to
do the patch routine so long as there is sufficient opportunity for
discussion.  For
small stuff - like  issue #33  - I don't think it is even necessary to
discuss it first
- so long as there are descriptive commit comments.

Steve:
Agreed with everything Philip said. I think the patch process started because I
complained about a commit before we'd had a chance to look at it. But I'm
really only
worried about commits that change APIs, not commits that add new classes or fix
obvious bugs. So yes, please go ahead and add them to the repository.

Original issue reported on code.google.com by pvogren@gmail.com on 1 May 2010 at 3:03

GoogleCodeExporter commented 8 years ago

Original comment by pvogren@gmail.com on 1 May 2010 at 5:36

GoogleCodeExporter commented 8 years ago
Ok - so I made the following changes to package structure per this issue:

component.JCasAnnotatorAdapter (from util)
pipeline.JCasIterable (from util)
pipeline.SimplePipeline (from util)
testing.factory.TokenFactory (from util)
testing.util.DisableLogging/HideOutput/TearDownUtil (from util)

I decided to leave AnnotationRetrieval and ReflectionUtil in the util package.  
I also left InitializeUtil alone which will be dealt with in issue #20

Original comment by pvogren@gmail.com on 10 Jun 2010 at 4:34