On 01/11/2007, at 10:46 AM, Joakim Erdfelt wrote:
Switching from boolean return on .applyPolicy() to throwing
exception, to gain better logging of why the transfer failed.
No need to change it now, but I don't really agree with this. I
don't agree with using exceptions for non-exceptional conditions
for readability reasons - particularly just to pass a string
around. IMO, better alternatives:
1) just log in the policy check and return true/false as before
2) return an object that has a "success" flag and a "reason"
string (essentially the same as an exception, but correctly using
the return type)
The flaw with that is that only having a boolean doesn't tell us
WHY something failed.
Also the log didn't make a distinction between config issue, normal
happy path, informational, and problem path.
Since this is a failure, throw an exception.
Made the code easier to read too. Lots of uncoding here.
But that's what option 2 is for - roughly same amount of code, has a
reason, but is still true/false return code. This isn't an
exceptional condition - you would expect things to fail policies all
the time when they are not updated, etc.
Anyway, it's just IMO, it obviously works just fine either way.
As for noise, the standard failure reasoning will easily be (10*
(proxyConnector.size()) more noisy than the configuration errors.
but that's getting adjusted in a separate issue, right?
Working on that now, the DefaultArchivaConfiguration.load()
mechanism has many configuration cleanup / sanity things in it.
Shame you got rid of the ConfigurationCleanup interface a while
back. would have made this easy to test individually instead of all
lumped together like it is now.
I'm not really sure what you are referring to here, but let's focus
on this some more after 1.0? I was just asking a question, I don't
think we want to much with configuration any further at this point as
the only issue I see open is one about clarity in the web interface.
- Brett
--
Brett Porter - [EMAIL PROTECTED]
Blog: http://www.devzuz.org/blogs/bporter/
