You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Jörg Schaible <jo...@gmx.de> on 2006/01/14 14:45:51 UTC
[id] Review before 1.0 (Summary)
Hi folks,
reviewing c-id I found more issues to resolve before releasing 1.0 final.
Since I already reported different complaints, I create here a summary of
anything already reported and all new issues.
1/ Concept of IdentifierGeneratorFactory
The IdentifierGeneratorFactory provides a easy-to-use concept for casual use
of id generators. Three elements are part of it:
abstract class IdentifierGeneratorFactory - defines the generator types
class DefaultIdentifierGeneratorFactory - delivers implementations
class IdentifierUtils - easy id generation using the default factory
the basic idea is, that a user can implement an own factory and provide it
with a system factory and it is automatically used as default in the
IdentifierUtils - so far so good.
The bad part in the concept is, that we have a variety of generator
implementations and all of them are quite different in their
initialization. Therefore has the IdentifierGeneratorFactory not only three
simple methods to generate the default generator for each (currently
supported) id type (String, Long and UUID), but has a variety of methods
tailored exactly for the ctors of the different generator implementations,
which makes this "factory" quite obscure.
Modifying a class with every new implementations is for me a kind of "code
smell". And the current code already gives evidence of this, since we have
new generator implementations, that cannot be accessed/initialized with the
factory. Additionally some methods have been added to support newer
generator implementations, but the appropriate methods to make use of the
generated ids are missing in the IdentifierUtils.
So we either complete those 3 classes for all generator implementations or
tailor them to a useful subset for the casual user:
interface IdentifierGeneratorFactory {
LongIdentifierGenerator longIDentifierGenerator();
StringIdentifierGenerator stringIDentifierGenerator();
UUIDIdentifierGenerator uuidIDentifierGenerator();
}
The question is, if a user *normally* requires in one application different
UUID genenerators, string generators or long generators? In all special
cases the generator instance can be generated directly or by an appropriate
environment (standard use case in IoC containers, where the implementation
and their initialization is configured anyway).
Proposal: Use as default a VersionFourGenerator, SessionIdGenerator and
LongGenerator. If he has the need for a different generator or a different
initialization, he can still provide easily an own factory and set the
system property.
2/ Exception handling for UUID 1 generator state
Currently c-id cannot be compiled with JDK 1.3, since it uses the ctor of
the RuntimeException with a causing Throwable. While this can easily
removed, the concepts leading to the situation must be reconsidered.
The UUID1 generation supports several implementations to persist the state
of such an generator. The State interface has a load and store method that
throw both just an arbitrary Exception, mainly because the underlying
implementation may have any kind of problems reading or storing the state
(XML parser, IO, DB, ...). The funny part is, that such an exception is
wrapped by the RuntimeEx from above and from the user's PoV he might get a
bad surprise calling nextIdentifier() on a UUID 1 generator ...
Proposal: Introduce a StatePerstistenceException derived from RTEx, that
might be thrown from the State implementations and document this at the
proper places in javadoc.
3/ xxxStateImpl classes
The javadoc of ReadOnlyResourceStateImpl implies that the implementation
tries to discover (well, use c-discovery) to load a file "uuid-[n].conf".
In reality it expects a system property (wrong name documented) set to a
file name that can be loaded as resource. All unit tests use a file
"uuid1.state", that is only available during the tests.
I am just wondering, if a default is simply missing or left by intension.
Also the derived ReadWriteFileStateImpl tries to write into the URL of the
resource ... which will fail badly if the file is within a jar.
4/ Copied c-codec classes in official API
To remove a dependency to c-codec the digest and hex utilities have been
copied to c-id, but they are now publicly available in the o.a.c.id
namespace.
Martin already proposed to move them to a package o.a.c.id.internal and
provide an appropriate package.html. As alternative we could try to make
them package accessible only and remove any unused functionality (they have
bad coverage reports because we only use view methods).
5/ Unit tests
A lot of unit tests in the c-id package have own suite methods and set an
own name for the test. While there are situations, where the usage of a
suite can be necessary, here it is definitely not. This not only results in
an inconsistent unit test report, but also prevents, that e.g. in Eclipse I
can simply double click the test to get to the appropriate code.
Proposal: Remove unnecessary suite methods and TestCase ctors taking a name.
6/ UUID version 2 + 3 + 5
Looking into the code if UUID.java I am not sure if 2+3 is already
supported. Some implementation details in nameUUIDFromString seem to
support also these versions, but there is no appropriate
VersionTwoGenerator or VersionThreeGenerator. What is the state of it?
7/ Exception signatures
Some methods have dubious exception signatures. Very bad is e.g. the ctor of
VersionOneGenerator. It claims to throw 4 different exceptions, but none of
them is actually thrown. Even worse, those exceptions are propagated to the
signature of a lot of callers ... up to the factory of issue 1.
While it is clear that I can clean this up, what is the common sense for
exception handling in c-id? Better wrap them up in an own RTEx derived
exception and document it? The IdentifierUtils don't declare any exceptions
and they were meant as use-and-forget tool ...
Please comment on the topics.
- Jörg
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [id] Review before 1.0 (Summary)
Posted by Jörg Schaible <jo...@gmx.de>.
Hi Stephen,
Stephen Colebourne wrote:
> As per the general thrust of Phil's mail, we should remove everything
> thats complex and not working and get a simple 1.0 out there. We can
> then grow from that point as and when the desire arises.
As answered to Phil, UUID is now part of c-id for two years and already used
in production. Nobody can guarantee for error-free code here anyway, but
all current issues with the code are not related to the algorithms, but to
exception handling and managing the internal (persistent) state of the
generator. This is not *that* problematic to clean it up.
[snip]
- Jörg
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [id] Review before 1.0 (Summary)
Posted by Stephen Colebourne <sc...@btopenworld.com>.
As per the general thrust of Phil's mail, we should remove everything
thats complex and not working and get a simple 1.0 out there. We can
then grow from that point as and when the desire arises.
Stephen
Phil Steitz wrote:
> On 1/14/06, Jörg Schaible <jo...@gmx.de> wrote:
>
>>Hi folks,
>>
>>reviewing c-id I found more issues to resolve before releasing 1.0 final.
>>Since I already reported different complaints, I create here a summary of
>>anything already reported and all new issues.
>>
>>1/ Concept of IdentifierGeneratorFactory
>>
>>The IdentifierGeneratorFactory provides a easy-to-use concept for casual use
>>of id generators. Three elements are part of it:
>>
>>abstract class IdentifierGeneratorFactory - defines the generator types
>>class DefaultIdentifierGeneratorFactory - delivers implementations
>>class IdentifierUtils - easy id generation using the default factory
>>
>>the basic idea is, that a user can implement an own factory and provide it
>>with a system factory and it is automatically used as default in the
>>IdentifierUtils - so far so good.
>>
>>The bad part in the concept is, that we have a variety of generator
>>implementations and all of them are quite different in their
>>initialization. Therefore has the IdentifierGeneratorFactory not only three
>>simple methods to generate the default generator for each (currently
>>supported) id type (String, Long and UUID), but has a variety of methods
>>tailored exactly for the ctors of the different generator implementations,
>>which makes this "factory" quite obscure.
>>
>
> Agreed. This is a consequence of the growth in the number of
> different generator types.
>
>
>>Modifying a class with every new implementations is for me a kind of "code
>>smell". And the current code already gives evidence of this, since we have
>>new generator implementations, that cannot be accessed/initialized with the
>>factory. Additionally some methods have been added to support newer
>>generator implementations, but the appropriate methods to make use of the
>>generated ids are missing in the IdentifierUtils.
>>
>
> Agreed again. The idea was to allow pluggable implemenations of the
> different generator types, but as the number of types increases, this
> gets unweildy.
>
>
>>So we either complete those 3 classes for all generator implementations or
>>tailor them to a useful subset for the casual user:
>>
>>interface IdentifierGeneratorFactory {
>> LongIdentifierGenerator longIDentifierGenerator();
>> StringIdentifierGenerator stringIDentifierGenerator();
>> UUIDIdentifierGenerator uuidIDentifierGenerator();
>>}
>
>
> But then you lose the distinction among the different subtypes. The
> question is, do we care about making the implementation of, say
> prefixAlphanumericGenerator pluggable (i.e., would anyone ever really
> want to provide a different factory implementation that returned a
> generator different from PrefixAlphaNumericGenerator from the one we
> provide? And if so, is the current setup the best way to support
> that. Answer is likely "no" to at least the second question.
>
>>The question is, if a user *normally* requires in one application different
>>UUID genenerators, string generators or long generators? In all special
>>cases the generator instance can be generated directly or by an appropriate
>>environment (standard use case in IoC containers, where the implementation
>>and their initialization is configured anyway).
>>
>>Proposal: Use as default a VersionFourGenerator, SessionIdGenerator and
>>LongGenerator. If he has the need for a different generator or a different
>>initialization, he can still provide easily an own factory and set the
>>system property.
>
>
> It might actually be better to dispense with
> IdentifierGeneratorFactory altogether, leaving the utils class in
> place, but having it directly instantiate provided impls. This would
> also allow us to drop the [discovery] dependency (with some additional
> work on uuid if we decidt to include this in the release - see below).
>
>>2/ Exception handling for UUID 1 generator state
>>
>>Currently c-id cannot be compiled with JDK 1.3, since it uses the ctor of
>>the RuntimeException with a causing Throwable. While this can easily
>>removed, the concepts leading to the situation must be reconsidered.
>>
>>The UUID1 generation supports several implementations to persist the state
>>of such an generator. The State interface has a load and store method that
>>throw both just an arbitrary Exception, mainly because the underlying
>>implementation may have any kind of problems reading or storing the state
>>(XML parser, IO, DB, ...). The funny part is, that such an exception is
>>wrapped by the RuntimeEx from above and from the user's PoV he might get a
>>bad surprise calling nextIdentifier() on a UUID 1 generator ...
>>
>>Proposal: Introduce a StatePerstistenceException derived from RTEx, that
>>might be thrown from the State implementations and document this at the
>>proper places in javadoc.
>>
>>3/ xxxStateImpl classes
>>
>>The javadoc of ReadOnlyResourceStateImpl implies that the implementation
>>tries to discover (well, use c-discovery) to load a file "uuid-[n].conf".
>>In reality it expects a system property (wrong name documented) set to a
>>file name that can be loaded as resource. All unit tests use a file
>>"uuid1.state", that is only available during the tests.
>>
>>I am just wondering, if a default is simply missing or left by intension.
>>
>>Also the derived ReadWriteFileStateImpl tries to write into the URL of the
>>resource ... which will fail badly if the file is within a jar.
>>
>>4/ Copied c-codec classes in official API
>>
>>To remove a dependency to c-codec the digest and hex utilities have been
>>copied to c-id, but they are now publicly available in the o.a.c.id
>>namespace.
>>
>>Martin already proposed to move them to a package o.a.c.id.internal and
>>provide an appropriate package.html. As alternative we could try to make
>>them package accessible only and remove any unused functionality (they have
>>bad coverage reports because we only use view methods).
>>
>>5/ Unit tests
>>
>>A lot of unit tests in the c-id package have own suite methods and set an
>>own name for the test. While there are situations, where the usage of a
>>suite can be necessary, here it is definitely not. This not only results in
>>an inconsistent unit test report, but also prevents, that e.g. in Eclipse I
>>can simply double click the test to get to the appropriate code.
>>
>>Proposal: Remove unnecessary suite methods and TestCase ctors taking a name.
>>
>>6/ UUID version 2 + 3 + 5
>>
>>Looking into the code if UUID.java I am not sure if 2+3 is already
>>supported. Some implementation details in nameUUIDFromString seem to
>>support also these versions, but there is no appropriate
>>VersionTwoGenerator or VersionThreeGenerator. What is the state of it?
>>
>>7/ Exception signatures
>>
>>Some methods have dubious exception signatures. Very bad is e.g. the ctor of
>>VersionOneGenerator. It claims to throw 4 different exceptions, but none of
>>them is actually thrown. Even worse, those exceptions are propagated to the
>>signature of a lot of callers ... up to the factory of issue 1.
>>
>>While it is clear that I can clean this up, what is the common sense for
>>exception handling in c-id? Better wrap them up in an own RTEx derived
>>exception and document it? The IdentifierUtils don't declare any exceptions
>>and they were meant as use-and-forget tool ...
>>
>
>
> One thing that I would like for us to consider is to omit the uuid
> generators from 1.0. There is a lot to do to get that package into
> releaseable state, including spec compliance review, and I am a little
> concerned with our ability to support it once released, as the
> original developer is no longer working on this code. I would be +1
> to cleaning up the rest of the package, promoting to proper and
> releasing a simple 1.0 without the uuid generators. I think the
> package is quite useful without the uuid functions and if there is
> sufficient community interest and support we can add the uuid stuff
> into a 1.1.
>
> Phil
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [id] Review before 1.0 (Summary)
Posted by Phil Steitz <ph...@gmail.com>.
On 1/15/06, Jörg Schaible <jo...@gmx.de> wrote:
> Hi Phil,
>
> Phil Steitz wrote:
>
> > <snip/>
> >>
> >> For me the question is, does a normal user really care about the
> >> specifics of an unique id. The most difference is the returned type of
> >> the id, because that may impact other parts of the code (e.g. DB).
> >
> > I think many apps will care about more than just the type as in string
> > or numeric, though I agree that the need for multiple different types
> > in one application will be rare. Some applications will want the id
> > to be random, even "secure" (e.g. session ids), some will want
> > sequential ids, etc. So I think the different types are significant.
> > The real questions are a) do we care to make different
> > implementations within type pluggable and b) do we want to provide
> > static methods that get ids of the different types directly as a
> > convenience. Answer to a) is probably leave it to the user and b) is
> > it is nice to have, but again gets unweildy with too many generators.
> > So, I guess I am in favor of simplifying down to just providing the
> > implementations - no factory, no utils class.
>
> OK. Fine with me. I'll drop it.
Lets make sure no one else is opposed. Stephen?
>
> >> > One thing that I would like for us to consider is to omit the uuid
> >> > generators from 1.0. There is a lot to do to get that package into
> >> > releaseable state, including spec compliance review, and I am a little
> >> > concerned with our ability to support it once released, as the
> >> > original developer is no longer working on this code.
> >>
> >> Uuuh! Cough! Bummer! When I look into the answers given to a lot of users
> >> over the last two years, they were always told, c-id is stable and
> >> reliable, we need just to close the minor issues left in Bugzilla and
> >> clean-up the docs.
> >
> > I don't ever recall anyone using the terms "stable" or "reliable" in
> > reference to the UUID code. The code from [lang] is certainly stable.
> > I did respond to one inquiry with the statement that "The main thing
> > standing in the
> > way of promotion to commons proper and a release is review and validation
> > of
> > the UUID code. " The "review and validation" (against the spec) of
> > the UUID code is a lot of work, as you can see.
>
> OK. You're right. Could not find anything else in the archives.
>
> >> I know quite some projects that use a snapshot of c-id
> >> to generate UUIDs in production (including some of mine).
> >
> > I don't know what we can do to make it more clear that people should
> > *not* depend on code in the commons sandbox, or any unreleased
> > snapshots.
>
> Release more often ? ;-)
>
Easier said than done, but I mostly agree with you. I say "mostly"
because some sandbox projects will never get the community and quality
necessary to get to a release, so releasing them too early would
actually be a bad thing, IMHO. That is why it is dangerous to depend
on them. There is sort of a chicken and egg problem here - some argue
that unless you release something you can't build community.
Another problem that we have in commons is that we really want to
maintain backward compatibility between major releases, so we have to
worrry a lot about getting the API right before the initial release.
We can fix bugs, improve implementations and add functionality in .x
releases, but the API we release intially is what we are more or less
stuck with for some time. This slows us down considerably compared to
projects whose core product is not an API.
In any case, I think we have critical mass to get [id] promoted and
released, and the API decisions left to make are fairly simple, so we
should agree on a release plan and get the work done. Your arguments
have convinced me at least that we should push forward to include the
uuid stuff. If you want to take a stab at a release plan and
volunteer to be release manager, feel free to go for it. I will help
with mechanics as necessary. Have a look at the releas plan wiki
pages out there for [scml] or old ones for [math], [digester],
[betwixt] et al as examples.
<snip/>
> >>
> >> Fact is, that UUIDs are part of c-id now for exactly two years and
> >> despite the fact, that c-id had no proper release, they are already used.
> >> Even I cannot use c-id-1.0, if it is without UUID support ... :-/
> >
> > This is a do-ocracy, so if you are willing to do the work to get all
> > of the UUID code into releasable state and verify compliance against
> > the spec, I am +1 to push forward. I just thought that we could get
> > promotion and a release out faster if we omitted this package from the
> > 1.0 plan.
>
> Fair enough. Well, in this case I would really wait until uuid is ready.
> Releasing c-id without uuid is IMHO worse than waiting longer for the
> proper release. I am willing to work on it, but I cannot say a lot about
> the compliance and if I really can handle that regarding my work load.
> Nevertheless my simple code review produced enough tasks that have to be
> done also for a final release. I'll just continue ...
I will help with the spec compliance and code review. Maybe others
can too. My time is also limited, but this is something I can do.
Thanks for picking this up.
Phil
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [id] Review before 1.0 (Summary)
Posted by Jörg Schaible <jo...@gmx.de>.
Hi Phil,
Phil Steitz wrote:
> <snip/>
>>
>> For me the question is, does a normal user really care about the
>> specifics of an unique id. The most difference is the returned type of
>> the id, because that may impact other parts of the code (e.g. DB).
>
> I think many apps will care about more than just the type as in string
> or numeric, though I agree that the need for multiple different types
> in one application will be rare. Some applications will want the id
> to be random, even "secure" (e.g. session ids), some will want
> sequential ids, etc. So I think the different types are significant.
> The real questions are a) do we care to make different
> implementations within type pluggable and b) do we want to provide
> static methods that get ids of the different types directly as a
> convenience. Answer to a) is probably leave it to the user and b) is
> it is nice to have, but again gets unweildy with too many generators.
> So, I guess I am in favor of simplifying down to just providing the
> implementations - no factory, no utils class.
OK. Fine with me. I'll drop it.
>> > One thing that I would like for us to consider is to omit the uuid
>> > generators from 1.0. There is a lot to do to get that package into
>> > releaseable state, including spec compliance review, and I am a little
>> > concerned with our ability to support it once released, as the
>> > original developer is no longer working on this code.
>>
>> Uuuh! Cough! Bummer! When I look into the answers given to a lot of users
>> over the last two years, they were always told, c-id is stable and
>> reliable, we need just to close the minor issues left in Bugzilla and
>> clean-up the docs.
>
> I don't ever recall anyone using the terms "stable" or "reliable" in
> reference to the UUID code. The code from [lang] is certainly stable.
> I did respond to one inquiry with the statement that "The main thing
> standing in the
> way of promotion to commons proper and a release is review and validation
> of
> the UUID code. " The "review and validation" (against the spec) of
> the UUID code is a lot of work, as you can see.
OK. You're right. Could not find anything else in the archives.
>> I know quite some projects that use a snapshot of c-id
>> to generate UUIDs in production (including some of mine).
>
> I don't know what we can do to make it more clear that people should
> *not* depend on code in the commons sandbox, or any unreleased
> snapshots.
Release more often ? ;-)
>> And look into
>> Bugzilla. We had not much issues about UUID and the last open one can be
>> easily solved by different implementations for the state. My issue list
>> targetted this area anyway (concerning Exception handling and the state
>> problem in this area).
>
> Again, the issue is validating the code and verifying spec compliance.
>
>>
>> > I would be +1
>> > to cleaning up the rest of the package, promoting to proper and
>> > releasing a simple 1.0 without the uuid generators. I think the
>> > package is quite useful without the uuid functions and if there is
>> > sufficient community interest and support we can add the uuid stuff
>> > into a 1.1.
>>
>> Fact is, that UUIDs are part of c-id now for exactly two years and
>> despite the fact, that c-id had no proper release, they are already used.
>> Even I cannot use c-id-1.0, if it is without UUID support ... :-/
>
> This is a do-ocracy, so if you are willing to do the work to get all
> of the UUID code into releasable state and verify compliance against
> the spec, I am +1 to push forward. I just thought that we could get
> promotion and a release out faster if we omitted this package from the
> 1.0 plan.
Fair enough. Well, in this case I would really wait until uuid is ready.
Releasing c-id without uuid is IMHO worse than waiting longer for the
proper release. I am willing to work on it, but I cannot say a lot about
the compliance and if I really can handle that regarding my work load.
Nevertheless my simple code review produced enough tasks that have to be
done also for a final release. I'll just continue ...
- Jörg
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [id] Review before 1.0 (Summary)
Posted by Phil Steitz <ph...@gmail.com>.
<snip/>
>
> For me the question is, does a normal user really care about the specifics
> of an unique id. The most difference is the returned type of the id,
> because that may impact other parts of the code (e.g. DB).
I think many apps will care about more than just the type as in string
or numeric, though I agree that the need for multiple different types
in one application will be rare. Some applications will want the id
to be random, even "secure" (e.g. session ids), some will want
sequential ids, etc. So I think the different types are significant.
The real questions are a) do we care to make different
implementations within type pluggable and b) do we want to provide
static methods that get ids of the different types directly as a
convenience. Answer to a) is probably leave it to the user and b) is
it is nice to have, but again gets unweildy with too many generators.
So, I guess I am in favor of simplifying down to just providing the
implementations - no factory, no utils class.
<snip/>
>
> > One thing that I would like for us to consider is to omit the uuid
> > generators from 1.0. There is a lot to do to get that package into
> > releaseable state, including spec compliance review, and I am a little
> > concerned with our ability to support it once released, as the
> > original developer is no longer working on this code.
>
> Uuuh! Cough! Bummer! When I look into the answers given to a lot of users
> over the last two years, they were always told, c-id is stable and
> reliable, we need just to close the minor issues left in Bugzilla and
> clean-up the docs.
I don't ever recall anyone using the terms "stable" or "reliable" in
reference to the UUID code. The code from [lang] is certainly stable.
I did respond to one inquiry with the statement that "The main thing
standing in the
way of promotion to commons proper and a release is review and validation of
the UUID code. " The "review and validation" (against the spec) of
the UUID code is a lot of work, as you can see.
> I know quite some projects that use a snapshot of c-id
> to generate UUIDs in production (including some of mine).
I don't know what we can do to make it more clear that people should
*not* depend on code in the commons sandbox, or any unreleased
snapshots.
> And look into
> Bugzilla. We had not much issues about UUID and the last open one can be
> easily solved by different implementations for the state. My issue list
> targetted this area anyway (concerning Exception handling and the state
> problem in this area).
Again, the issue is validating the code and verifying spec compliance.
>
> > I would be +1
> > to cleaning up the rest of the package, promoting to proper and
> > releasing a simple 1.0 without the uuid generators. I think the
> > package is quite useful without the uuid functions and if there is
> > sufficient community interest and support we can add the uuid stuff
> > into a 1.1.
>
> Fact is, that UUIDs are part of c-id now for exactly two years and despite
> the fact, that c-id had no proper release, they are already used. Even I
> cannot use c-id-1.0, if it is without UUID support ... :-/
This is a do-ocracy, so if you are willing to do the work to get all
of the UUID code into releasable state and verify compliance against
the spec, I am +1 to push forward. I just thought that we could get
promotion and a release out faster if we omitted this package from the
1.0 plan.
Phil
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [id] Review before 1.0 (Summary)
Posted by Jörg Schaible <jo...@gmx.de>.
Hi Phil,
comments in line again ...
Phil Steitz wrote:
> On 1/14/06, Jörg Schaible <jo...@gmx.de> wrote:
>> Hi folks,
>>
>> reviewing c-id I found more issues to resolve before releasing 1.0 final.
>> Since I already reported different complaints, I create here a summary of
>> anything already reported and all new issues.
>>
>> 1/ Concept of IdentifierGeneratorFactory
>>
>> The IdentifierGeneratorFactory provides a easy-to-use concept for casual
>> use of id generators. Three elements are part of it:
>>
>> abstract class IdentifierGeneratorFactory - defines the generator types
>> class DefaultIdentifierGeneratorFactory - delivers implementations
>> class IdentifierUtils - easy id generation using the default factory
>>
>> the basic idea is, that a user can implement an own factory and provide
>> it with a system factory and it is automatically used as default in the
>> IdentifierUtils - so far so good.
>>
>> The bad part in the concept is, that we have a variety of generator
>> implementations and all of them are quite different in their
>> initialization. Therefore has the IdentifierGeneratorFactory not only
>> three simple methods to generate the default generator for each
>> (currently supported) id type (String, Long and UUID), but has a variety
>> of methods tailored exactly for the ctors of the different generator
>> implementations, which makes this "factory" quite obscure.
>>
> Agreed. This is a consequence of the growth in the number of
> different generator types.
>
>> Modifying a class with every new implementations is for me a kind of
>> "code smell". And the current code already gives evidence of this, since
>> we have new generator implementations, that cannot be
>> accessed/initialized with the factory. Additionally some methods have
>> been added to support newer generator implementations, but the
>> appropriate methods to make use of the generated ids are missing in the
>> IdentifierUtils.
>>
> Agreed again. The idea was to allow pluggable implemenations of the
> different generator types, but as the number of types increases, this
> gets unweildy.
>
>> So we either complete those 3 classes for all generator implementations
>> or tailor them to a useful subset for the casual user:
>>
>> interface IdentifierGeneratorFactory {
>> LongIdentifierGenerator longIDentifierGenerator();
>> StringIdentifierGenerator stringIDentifierGenerator();
>> UUIDIdentifierGenerator uuidIDentifierGenerator();
>> }
>
> But then you lose the distinction among the different subtypes. The
> question is, do we care about making the implementation of, say
> prefixAlphanumericGenerator pluggable (i.e., would anyone ever really
> want to provide a different factory implementation that returned a
> generator different from PrefixAlphaNumericGenerator from the one we
> provide? And if so, is the current setup the best way to support
> that. Answer is likely "no" to at least the second question.
For me the question is, does a normal user really care about the specifics
of an unique id. The most difference is the returned type of the id,
because that may impact other parts of the code (e.g. DB). If we provide a
sensible default for each type, it might be enough. And then it is really
simple for everyone to provide a different generator for each of the types.
>> The question is, if a user *normally* requires in one application
>> different
>> UUID genenerators, string generators or long generators? In all special
>> cases the generator instance can be generated directly or by an
>> appropriate environment (standard use case in IoC containers, where the
>> implementation and their initialization is configured anyway).
>>
>> Proposal: Use as default a VersionFourGenerator, SessionIdGenerator and
>> LongGenerator. If he has the need for a different generator or a
>> different initialization, he can still provide easily an own factory and
>> set the system property.
>
> It might actually be better to dispense with
> IdentifierGeneratorFactory altogether, leaving the utils class in
> place, but having it directly instantiate provided impls.
Well - as pico guy - the factory is for me some kind of poor man's IoC
approach, but it might have a place if it is much simpler. But even for the
utils class the argumentation is similar: Do we need a static instance for
every generator type and their different initializations and an accessor
method for each nextIdentifier() method? You have the same problem here:
Add a new generator implementation and you have to add one or more
appropriate methods and static instances to the utils class. Use the "type"
approach from above and you have 3 instances with 3 methods accessing the
specified factory.
> This would
> also allow us to drop the [discovery] dependency (with some additional
> work on uuid if we decidt to include this in the release - see below).
See also below ...
[snip issues 2-7 for which I hope also to receive some comments one time
<g>]
> One thing that I would like for us to consider is to omit the uuid
> generators from 1.0. There is a lot to do to get that package into
> releaseable state, including spec compliance review, and I am a little
> concerned with our ability to support it once released, as the
> original developer is no longer working on this code.
Uuuh! Cough! Bummer! When I look into the answers given to a lot of users
over the last two years, they were always told, c-id is stable and
reliable, we need just to close the minor issues left in Bugzilla and
clean-up the docs. I know quite some projects that use a snapshot of c-id
to generate UUIDs in production (including some of mine). And look into
Bugzilla. We had not much issues about UUID and the last open one can be
easily solved by different implementations for the state. My issue list
targetted this area anyway (concerning Exception handling and the state
problem in this area).
> I would be +1
> to cleaning up the rest of the package, promoting to proper and
> releasing a simple 1.0 without the uuid generators. I think the
> package is quite useful without the uuid functions and if there is
> sufficient community interest and support we can add the uuid stuff
> into a 1.1.
Fact is, that UUIDs are part of c-id now for exactly two years and despite
the fact, that c-id had no proper release, they are already used. Even I
cannot use c-id-1.0, if it is without UUID support ... :-/
- Jörg
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [id] Review before 1.0 (Summary)
Posted by Phil Steitz <ph...@gmail.com>.
On 1/14/06, Jörg Schaible <jo...@gmx.de> wrote:
> Hi folks,
>
> reviewing c-id I found more issues to resolve before releasing 1.0 final.
> Since I already reported different complaints, I create here a summary of
> anything already reported and all new issues.
>
> 1/ Concept of IdentifierGeneratorFactory
>
> The IdentifierGeneratorFactory provides a easy-to-use concept for casual use
> of id generators. Three elements are part of it:
>
> abstract class IdentifierGeneratorFactory - defines the generator types
> class DefaultIdentifierGeneratorFactory - delivers implementations
> class IdentifierUtils - easy id generation using the default factory
>
> the basic idea is, that a user can implement an own factory and provide it
> with a system factory and it is automatically used as default in the
> IdentifierUtils - so far so good.
>
> The bad part in the concept is, that we have a variety of generator
> implementations and all of them are quite different in their
> initialization. Therefore has the IdentifierGeneratorFactory not only three
> simple methods to generate the default generator for each (currently
> supported) id type (String, Long and UUID), but has a variety of methods
> tailored exactly for the ctors of the different generator implementations,
> which makes this "factory" quite obscure.
>
Agreed. This is a consequence of the growth in the number of
different generator types.
> Modifying a class with every new implementations is for me a kind of "code
> smell". And the current code already gives evidence of this, since we have
> new generator implementations, that cannot be accessed/initialized with the
> factory. Additionally some methods have been added to support newer
> generator implementations, but the appropriate methods to make use of the
> generated ids are missing in the IdentifierUtils.
>
Agreed again. The idea was to allow pluggable implemenations of the
different generator types, but as the number of types increases, this
gets unweildy.
> So we either complete those 3 classes for all generator implementations or
> tailor them to a useful subset for the casual user:
>
> interface IdentifierGeneratorFactory {
> LongIdentifierGenerator longIDentifierGenerator();
> StringIdentifierGenerator stringIDentifierGenerator();
> UUIDIdentifierGenerator uuidIDentifierGenerator();
> }
But then you lose the distinction among the different subtypes. The
question is, do we care about making the implementation of, say
prefixAlphanumericGenerator pluggable (i.e., would anyone ever really
want to provide a different factory implementation that returned a
generator different from PrefixAlphaNumericGenerator from the one we
provide? And if so, is the current setup the best way to support
that. Answer is likely "no" to at least the second question.
>
> The question is, if a user *normally* requires in one application different
> UUID genenerators, string generators or long generators? In all special
> cases the generator instance can be generated directly or by an appropriate
> environment (standard use case in IoC containers, where the implementation
> and their initialization is configured anyway).
>
> Proposal: Use as default a VersionFourGenerator, SessionIdGenerator and
> LongGenerator. If he has the need for a different generator or a different
> initialization, he can still provide easily an own factory and set the
> system property.
It might actually be better to dispense with
IdentifierGeneratorFactory altogether, leaving the utils class in
place, but having it directly instantiate provided impls. This would
also allow us to drop the [discovery] dependency (with some additional
work on uuid if we decidt to include this in the release - see below).
>
> 2/ Exception handling for UUID 1 generator state
>
> Currently c-id cannot be compiled with JDK 1.3, since it uses the ctor of
> the RuntimeException with a causing Throwable. While this can easily
> removed, the concepts leading to the situation must be reconsidered.
>
> The UUID1 generation supports several implementations to persist the state
> of such an generator. The State interface has a load and store method that
> throw both just an arbitrary Exception, mainly because the underlying
> implementation may have any kind of problems reading or storing the state
> (XML parser, IO, DB, ...). The funny part is, that such an exception is
> wrapped by the RuntimeEx from above and from the user's PoV he might get a
> bad surprise calling nextIdentifier() on a UUID 1 generator ...
>
> Proposal: Introduce a StatePerstistenceException derived from RTEx, that
> might be thrown from the State implementations and document this at the
> proper places in javadoc.
>
> 3/ xxxStateImpl classes
>
> The javadoc of ReadOnlyResourceStateImpl implies that the implementation
> tries to discover (well, use c-discovery) to load a file "uuid-[n].conf".
> In reality it expects a system property (wrong name documented) set to a
> file name that can be loaded as resource. All unit tests use a file
> "uuid1.state", that is only available during the tests.
>
> I am just wondering, if a default is simply missing or left by intension.
>
> Also the derived ReadWriteFileStateImpl tries to write into the URL of the
> resource ... which will fail badly if the file is within a jar.
>
> 4/ Copied c-codec classes in official API
>
> To remove a dependency to c-codec the digest and hex utilities have been
> copied to c-id, but they are now publicly available in the o.a.c.id
> namespace.
>
> Martin already proposed to move them to a package o.a.c.id.internal and
> provide an appropriate package.html. As alternative we could try to make
> them package accessible only and remove any unused functionality (they have
> bad coverage reports because we only use view methods).
>
> 5/ Unit tests
>
> A lot of unit tests in the c-id package have own suite methods and set an
> own name for the test. While there are situations, where the usage of a
> suite can be necessary, here it is definitely not. This not only results in
> an inconsistent unit test report, but also prevents, that e.g. in Eclipse I
> can simply double click the test to get to the appropriate code.
>
> Proposal: Remove unnecessary suite methods and TestCase ctors taking a name.
>
> 6/ UUID version 2 + 3 + 5
>
> Looking into the code if UUID.java I am not sure if 2+3 is already
> supported. Some implementation details in nameUUIDFromString seem to
> support also these versions, but there is no appropriate
> VersionTwoGenerator or VersionThreeGenerator. What is the state of it?
>
> 7/ Exception signatures
>
> Some methods have dubious exception signatures. Very bad is e.g. the ctor of
> VersionOneGenerator. It claims to throw 4 different exceptions, but none of
> them is actually thrown. Even worse, those exceptions are propagated to the
> signature of a lot of callers ... up to the factory of issue 1.
>
> While it is clear that I can clean this up, what is the common sense for
> exception handling in c-id? Better wrap them up in an own RTEx derived
> exception and document it? The IdentifierUtils don't declare any exceptions
> and they were meant as use-and-forget tool ...
>
One thing that I would like for us to consider is to omit the uuid
generators from 1.0. There is a lot to do to get that package into
releaseable state, including spec compliance review, and I am a little
concerned with our ability to support it once released, as the
original developer is no longer working on this code. I would be +1
to cleaning up the rest of the package, promoting to proper and
releasing a simple 1.0 without the uuid generators. I think the
package is quite useful without the uuid functions and if there is
sufficient community interest and support we can add the uuid stuff
into a 1.1.
Phil
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [id] Review before 1.0 (Summary)
Posted by Phil Steitz <ph...@gmail.com>.
On 1/15/06, Jörg Schaible <jo...@gmx.de> wrote:
> Phil Steitz wrote:
>
> > On 1/14/06, Michael Heuer <he...@acm.org> wrote:
> >>
> >> Jörg Schaible wrote:
> >>
> >> > 8/ Prefix generators
> >> >
> >> > We have 3 generators, that add a prefix to the generated id. All 3
> >> > classes to mainly the same for 3 different StringIdentifierGenerators.
> >> >
> >> > Proposal: Since we have a lot more StringIdentifierGenrators (e.g. the
> >> > UUIDIdentifierGenerators a StringIdentifiers too), I would refactore
> >> > this 3 classes into a wrapper in a separate package o.a.c.id.wrapper
> >> > and delegate
> >
> >> > to an arbitrary StringIdentifierGenerator implementation:
> >> >
> >> > class PrefixStringIdentifierGenerator implements
> >> > StringIdentifierGenerator {
> >> > PrefixStringIdentifierGenerator(StringIdentifierGenerator
> >> > delegee) {
> >> > this.delegee = delegee;
> >> > }
> >> > ...
> >> > }
> >>
> >> How about a single class CompoundStringIdentifierGenerator that can use
> >> the delegate as any one of prefix, middle, postfix?
> >>
> >
> > +1 - I think we discussed something like this before. More generally,
> > we could support arbitrary concatenations using an API like
> > o.a.c.Collections.ChainedTransformer
> >
> <http://jakarta.apache.org/commons/collections/api-release/org/apache/commons/collections/functors/ChainedTransformer.html>
>
> OK, I'll have a look.
Assuming you and others are OK with this approach, I will pick up the
implementation and test cases, or review / commit patches.
>
> >> And may I add
> >>
> >> 10/ Serial generators implement Serializable?
> >
> > +1
>
> Good idea. Most of the generators could implement it.
I will handle this as well, or review / commit patches as they come in.
Phil
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [id] Review before 1.0 (Summary)
Posted by Jörg Schaible <jo...@gmx.de>.
Phil Steitz wrote:
> On 1/14/06, Michael Heuer <he...@acm.org> wrote:
>>
>> Jörg Schaible wrote:
>>
>> > 8/ Prefix generators
>> >
>> > We have 3 generators, that add a prefix to the generated id. All 3
>> > classes to mainly the same for 3 different StringIdentifierGenerators.
>> >
>> > Proposal: Since we have a lot more StringIdentifierGenrators (e.g. the
>> > UUIDIdentifierGenerators a StringIdentifiers too), I would refactore
>> > this 3 classes into a wrapper in a separate package o.a.c.id.wrapper
>> > and delegate
>
>> > to an arbitrary StringIdentifierGenerator implementation:
>> >
>> > class PrefixStringIdentifierGenerator implements
>> > StringIdentifierGenerator {
>> > PrefixStringIdentifierGenerator(StringIdentifierGenerator
>> > delegee) {
>> > this.delegee = delegee;
>> > }
>> > ...
>> > }
>>
>> How about a single class CompoundStringIdentifierGenerator that can use
>> the delegate as any one of prefix, middle, postfix?
>>
>
> +1 - I think we discussed something like this before. More generally,
> we could support arbitrary concatenations using an API like
> o.a.c.Collections.ChainedTransformer
>
<http://jakarta.apache.org/commons/collections/api-release/org/apache/commons/collections/functors/ChainedTransformer.html>
OK, I'll have a look.
>> And may I add
>>
>> 10/ Serial generators implement Serializable?
>
> +1
Good idea. Most of the generators could implement it.
- Jörg
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [id] Review before 1.0 (Summary)
Posted by Phil Steitz <ph...@gmail.com>.
On 1/14/06, Michael Heuer <he...@acm.org> wrote:
>
> Jörg Schaible wrote:
>
> > 8/ Prefix generators
> >
> > We have 3 generators, that add a prefix to the generated id. All 3 classes
> > to mainly the same for 3 different StringIdentifierGenerators.
> >
> > Proposal: Since we have a lot more StringIdentifierGenrators (e.g. the
> > UUIDIdentifierGenerators a StringIdentifiers too), I would refactore this 3
> > classes into a wrapper in a separate package o.a.c.id.wrapper and delegate
> > to an arbitrary StringIdentifierGenerator implementation:
> >
> > class PrefixStringIdentifierGenerator implements StringIdentifierGenerator {
> > PrefixStringIdentifierGenerator(StringIdentifierGenerator delegee) {
> > this.delegee = delegee;
> > }
> > ...
> > }
>
> How about a single class CompoundStringIdentifierGenerator that can use
> the delegate as any one of prefix, middle, postfix?
>
+1 - I think we discussed something like this before. More generally,
we could support arbitrary concatenations using an API like
o.a.c.Collections.ChainedTransformer
<http://jakarta.apache.org/commons/collections/api-release/org/apache/commons/collections/functors/ChainedTransformer.html>
>
> And may I add
>
> 10/ Serial generators implement Serializable?
+1
Phil
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [id] Review before 1.0 (Summary)
Posted by Michael Heuer <he...@acm.org>.
J�rg Schaible wrote:
> 8/ Prefix generators
>
> We have 3 generators, that add a prefix to the generated id. All 3 classes
> to mainly the same for 3 different StringIdentifierGenerators.
>
> Proposal: Since we have a lot more StringIdentifierGenrators (e.g. the
> UUIDIdentifierGenerators a StringIdentifiers too), I would refactore this 3
> classes into a wrapper in a separate package o.a.c.id.wrapper and delegate
> to an arbitrary StringIdentifierGenerator implementation:
>
> class PrefixStringIdentifierGenerator implements StringIdentifierGenerator {
> PrefixStringIdentifierGenerator(StringIdentifierGenerator delegee) {
> this.delegee = delegee;
> }
> ...
> }
How about a single class CompoundStringIdentifierGenerator that can use
the delegate as any one of prefix, middle, postfix?
And may I add
10/ Serial generators implement Serializable?
michael
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [id] Review before 1.0 (Summary)
Posted by Jörg Schaible <jo...@gmx.de>.
Hi fellows,
two of the issues have already be solved (see inline comments), but I have
two new ones, see at the bottom.
Jörg Schaible wrote:
[snip]
> 3/ xxxStateImpl classes
>
> The javadoc of ReadOnlyResourceStateImpl implies that the implementation
> tries to discover (well, use c-discovery) to load a file "uuid-[n].conf".
> In reality it expects a system property (wrong name documented) set to a
> file name that can be loaded as resource. All unit tests use a file
> "uuid1.state", that is only available during the tests.
>
> I am just wondering, if a default is simply missing or left by intension.
This one is answered by the UUID doc of c-id.
> Also the derived ReadWriteFileStateImpl tries to write into the URL of the
> resource ... which will fail badly if the file is within a jar.
This part is addressed in the only current open Bugzilla issue with a
comment from Tim.
[snip]
> 5/ Unit tests
>
> A lot of unit tests in the c-id package have own suite methods and set an
> own name for the test. While there are situations, where the usage of a
> suite can be necessary, here it is definitely not. This not only results
> in an inconsistent unit test report, but also prevents, that e.g. in
> Eclipse I can simply double click the test to get to the appropriate code.
>
> Proposal: Remove unnecessary suite methods and TestCase ctors taking a
> name.
Phil agreed on this in private mail. Done.
[snip]
8/ Prefix generators
We have 3 generators, that add a prefix to the generated id. All 3 classes
to mainly the same for 3 different StringIdentifierGenerators.
Proposal: Since we have a lot more StringIdentifierGenrators (e.g. the
UUIDIdentifierGenerators a StringIdentifiers too), I would refactore this 3
classes into a wrapper in a separate package o.a.c.id.wrapper and delegate
to an arbitrary StringIdentifierGenerator implementation:
class PrefixStringIdentifierGenerator implements StringIdentifierGenerator {
PrefixStringIdentifierGenerator(StringIdentifierGenerator delegee) {
this.delegee = delegee;
}
...
}
9/ Wrappable genrators
The generators in the o.a.id.serial package generate ids in a sequence. A
lot of them can start over if the maximum value is reached based on the
"wrap" flag.
Proposal: Introduce a Wrappable interface, that is already implemented by
those ones:
interface Wrappable {
boolean isWrapping();
void setWrap(boolean wrap);
}
Comments?
- Jörg
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [id] Review before 1.0 (Summary)
Posted by Tim OBrien <to...@discursive.com>.
--- Jörg Schaible <jo...@gmx.de> wrote:
> Hi Tim,
>
> Tim OBrien wrote:
>
> > Good summary, comments inline...
> >
> > --- Jörg Schaible <jo...@gmx.de> wrote:
> >
> >> 4/ Copied c-codec classes in official API
> >>
> >> To remove a dependency to c-codec the digest and hex utilities have been
> >> copied to c-id, but they are now publicly available in the o.a.c.id
> >> namespace.
> >>
> >> Martin already proposed to move them to a package o.a.c.id.internal and
> >> provide an appropriate package.html. As alternative we could try to make
> >> them package accessible only and remove any unused functionality (they
> >> have bad coverage reports because we only use view methods).
> >
> > +1 with moving into an internal package with a package.html,
> >
> > Of two minds on the coverage issue, one one hand the class is well tested
> > over in codec so you
> > could just trust that the class is well tested. But, could you also just
> > copy the unit tests from
> > c-codec and add them into c-id. I don't see a huge problem there as long
> > as there is sufficient notice in the classes to the effect of "DON'T
> > CHANGE ME HERE, CHANGE ME IN COMMONS CODEC".
>
> If you remove the unused code, you have no tests, but coverage :)
>
Definitely, I see the point, I was just concerned that you might be removing something that might
eventually be needed, but if you are covering c-id code you know exactly what is needed and what
isn't.
> Also it is much less encouraging for people to use these classes, if you
> state that you have only a partial copy of the original ...
>
Definitely.
> - Jörg
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [id] Review before 1.0 (Summary)
Posted by Jörg Schaible <jo...@gmx.de>.
Hi Tim,
Tim OBrien wrote:
> Good summary, comments inline...
>
> --- Jörg Schaible <jo...@gmx.de> wrote:
>
>> 4/ Copied c-codec classes in official API
>>
>> To remove a dependency to c-codec the digest and hex utilities have been
>> copied to c-id, but they are now publicly available in the o.a.c.id
>> namespace.
>>
>> Martin already proposed to move them to a package o.a.c.id.internal and
>> provide an appropriate package.html. As alternative we could try to make
>> them package accessible only and remove any unused functionality (they
>> have bad coverage reports because we only use view methods).
>
> +1 with moving into an internal package with a package.html,
>
> Of two minds on the coverage issue, one one hand the class is well tested
> over in codec so you
> could just trust that the class is well tested. But, could you also just
> copy the unit tests from
> c-codec and add them into c-id. I don't see a huge problem there as long
> as there is sufficient notice in the classes to the effect of "DON'T
> CHANGE ME HERE, CHANGE ME IN COMMONS CODEC".
If you remove the unused code, you have no tests, but coverage :)
Also it is much less encouraging for people to use these classes, if you
state that you have only a partial copy of the original ...
- Jörg
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [id] Review before 1.0 (Summary)
Posted by Jörg Schaible <jo...@gmx.de>.
Hi Stephen,
Stephen Colebourne wrote:
> Tim OBrien wrote:
>> --- Jörg Schaible <jo...@gmx.de> wrote:
>>>4/ Copied c-codec classes in official API
>>>
>>>To remove a dependency to c-codec the digest and hex utilities have been
>>>copied to c-id, but they are now publicly available in the o.a.c.id
>>>namespace.
[snip]
>> +1 with moving into an internal package with a package.html,
>
> I strongly prefer using package scope if at all possible, and
> referencing the svn revision id that the code was copied from.
Well, there's no problem in creating a copy with svn from the current code
code, but this does not affect the removal of unused code in the copy. A
later 3-way-merge can update the remaining code always. Concerning package
scope I will have to take a closer look, since the functionality is used in
two c-id packages.
- Jörg
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [id] Review before 1.0 (Summary)
Posted by Stephen Colebourne <sc...@btopenworld.com>.
Tim OBrien wrote:
> --- Jörg Schaible <jo...@gmx.de> wrote:
>>4/ Copied c-codec classes in official API
>>
>>To remove a dependency to c-codec the digest and hex utilities have been
>>copied to c-id, but they are now publicly available in the o.a.c.id
>>namespace.
>>
>>Martin already proposed to move them to a package o.a.c.id.internal and
>>provide an appropriate package.html. As alternative we could try to make
>>them package accessible only and remove any unused functionality (they have
>>bad coverage reports because we only use view methods).
>
>
> +1 with moving into an internal package with a package.html,
I strongly prefer using package scope if at all possible, and
referencing the svn revision id that the code was copied from.
Stephen
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [id] Review before 1.0 (Summary)
Posted by Tim OBrien <to...@discursive.com>.
Good summary, comments inline...
--- Jörg Schaible <jo...@gmx.de> wrote:
> 4/ Copied c-codec classes in official API
>
> To remove a dependency to c-codec the digest and hex utilities have been
> copied to c-id, but they are now publicly available in the o.a.c.id
> namespace.
>
> Martin already proposed to move them to a package o.a.c.id.internal and
> provide an appropriate package.html. As alternative we could try to make
> them package accessible only and remove any unused functionality (they have
> bad coverage reports because we only use view methods).
+1 with moving into an internal package with a package.html,
Of two minds on the coverage issue, one one hand the class is well tested over in codec so you
could just trust that the class is well tested. But, could you also just copy the unit tests from
c-codec and add them into c-id. I don't see a huge problem there as long as there is sufficient
notice in the classes to the effect of "DON'T CHANGE ME HERE, CHANGE ME IN COMMONS CODEC".
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org