You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by "Daniel F. Savarese" <df...@savarese.org> on 2004/01/05 18:17:28 UTC
Re: [net] checked in parser factory implementation
In message <20...@javactivity.org>, steve cohen writes:
>Problem is, the current implementations of FTPFileEntryParser, also, for
>reasons of backward compatibility, implement FTPFileListParser. I don't
>remember exactly why I did that but there was a reason. But the problem is
>that deprecating FTPFileListParser will deprecate every parser currently in
>the package.
I don't have a comment on this one yet as I have to look at the code
in more detail. I'm in favor of deprecating the old stuff, but figuring
out the right way to do it isn't immediately obvious. (Actually,
scratch that, now that I've finished writing this email, I have comments
at the end).
>parser and specification of parser either by classname or key ("UNIX", "VMS",
>etc.) will go a long way to making this easier to deal with. The key is to
>now use the new FTPClient.getFileList() methods in place of
>FTPClient.listFiles(). They can use the autodetect feature and never get
>near the question of which parser interface to use. Even if specifying by
>key, all these internals are handled for them. It really should be lots
>better. If anything needs deprecation at this point, I would say it might be
>FTPClient.listFiles().
I disagree slightly (it's actually agreement, but with a naming
difference) and have some suggested code changes (I can make them if
there's agreement). First, the getFileList methods should be
named listFiles() as that's what they're replacing. We can
deprecate the old listFile methods that take FTPFileListParser
arguments and reimplement listFiles() and listFiles(String pathname) in
terms of the new stuff using autodection (removing
getFileList(String parserKey) since the signatures conflict and the
common case will be to use autodetection anyway). Second, we shouldn't
create an FTPFileListParserFactory on every call to getFileList. Instead,
FTPClient should maintain a reference to an FTPFileEntryParserFactory.
This reference should be configurable by the user with a setter
method. That allows API users to change the factory implementation.
Third, we shouldn't set the default factory based on a property.
Once we add the setter method, there is no need for the property.
In general, it's better to let applications handle their configuration
through properties and leave the library free of dependencies on property
values. Which factory to use is an application-level decision (e.g.,
an ant task can define a property that changes the factory for its
purposes), not a library-level decision.
Jeffrey Brekke writes:
>list parsers, and/or implementing the existing list parsers with an
>implementation that uses the entry parsers. That way we are parsing
>with one implementation?
That should be probably be our first step toward deprecation so the
parsers behave consistently. In the 1.2 release we can deprecate the
old listFiles methods presuming we agree to keep the method name for
the files (the class method names currently mirror as closely as possible
the FTP commands they implement). We're stuck by the Commons
versioning/release rules regarding when we can remove the old stuff.
It has to stay in all of the 1.x releases. Otherwise, I'd say that in
1.2 we announce that in 1.3 FTPFileListParserImpl will no longer
implement FTPFileListParser and deprecate the interface in 1.2, removing
it in 1.4 along with the old listFiles() methods. We'll just have to
wait until 2.0. I just tested it, and if we deprecate FTPFileListParser
now, it only affects the javadocs for FTPFileListParser. I think we
can live with any compiler warnings saying that you're using a
deprecated interface as long as we document this. With a J2SE 1.4
compile, you only get warnings for the listFiles methods,
DefaultFTPFileLister (which we also need to deprecate), and
FTPFileListParserImpl, but none of the FTPFileListParserImpl subclasses.
So I propose deprecation in 1.2 and removal in 2.0.
I'd also like to tweak the build files before a 1.2 release because,
given our limited free time, it's excessive to trigger unit tests every
time you build the javadocs since the unit tests don't test anything
in the javadocs. It's just that the unit tests take much so long on
my development box (I'm still using sub-1GHz processors), I need
a way to bypass them when it's not necessary to run them. It's a
tradeoff between productivity and comprehensiveness.
daniel
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [net] checked in parser factory implementation
Posted by "Jeffrey D. Brekke" <jb...@wi.rr.com>.
>>>>> On Mon, 05 Jan 2004 12:17:28 -0500, "Daniel F. Savarese" <df...@savarese.org> said:
[SNIPPED]
> I'd also like to tweak the build files before a 1.2 release because,
> given our limited free time, it's excessive to trigger unit tests
> every time you build the javadocs since the unit tests don't test
> anything in the javadocs. It's just that the unit tests take much
> so long on my development box (I'm still using sub-1GHz processors),
> I need a way to bypass them when it's not necessary to run them.
> It's a tradeoff between productivity and comprehensiveness.
[SNIPPED]
I've submitted a patch to the maven-ant plugin to resolve this. In
the meantime I've committed a newly regenerated build.xml with the
changes. Maven does not execute the tests on a javadoc goal.
--
=====================================================================
Jeffrey D. Brekke jbrekke@wi.rr.com
Wisconsin, USA brekke@apache.org
ekkerbj@yahoo.com
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [net] checked in parser factory implementation
Posted by steve cohen <sc...@javactivity.org>.
On Monday 05 January 2004 10:23 pm, Jeffrey D. Brekke wrote:
> >>>>> On Mon, 05 Jan 2004 21:34:01 -0600, jbrekke@wi.rr.com (Jeffrey D.
> >>>>> Brekke) said:
> >>>>>
> >>>>> On Mon, 05 Jan 2004 12:07:28 -0600, steve cohen
> >>>>> <sc...@javactivity.org> said:
> >
> > [SNIPPED]
> >
> >> 4) rename getFileList(String parserKey, String pathname) to
> >> listFiles(String parserKey, String pathname). Even though
> >> autodetection will be the primary use case, it already will not
> >> work for every system (see EnterpriseUnix) and there must be some
> >> way around it. This is that way.
> >
> > [SNIPPED]
> >
> > I don't believe this method needs to be public. It seems that the
> > listFiles(FTPFileEntryParser parser, String pathname) is sufficient.
> > A fully-qualified classname is required for the EnterpriseUnix use
> > case anyway, right? And if you want to use autodetection, just use
> > the listFiles(String pathname) version.
>
> I meant to say that createFiles(String pathname, FTPFileEntryParser parser)
> is sufficient for this case.
>
I see your point, but I don't agree for a couple of reasons having to do with
the ant use case with a parser that can't be detected by autodetect, such as
Enterprise Unix. If we were going to use the createFileList() (I think
that's what you really meant, no?) then we wind up exposing even more of the
internals involved with iterating that list. (We might want to think about
restricting the access of some of those methods!). And the ant task would
then become responsible for instantiating the parser. Not a big deal, just
call newInstance(), but that still involves ant knowing more about
commons-net internals than I think it should. On balance, passing in a
string seems a little cleaner.
I do sympathize with your wanting to simplify the outside interfaces, but I
think, on balance, that having this method public really is the simpler way.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [net] checked in parser factory implementation
Posted by "Jeffrey D. Brekke" <jb...@wi.rr.com>.
>>>>> On Mon, 05 Jan 2004 21:34:01 -0600, jbrekke@wi.rr.com (Jeffrey D. Brekke) said:
>>>>> On Mon, 05 Jan 2004 12:07:28 -0600, steve cohen <sc...@javactivity.org> said:
> [SNIPPED]
>> 4) rename getFileList(String parserKey, String pathname) to
>> listFiles(String parserKey, String pathname). Even though
>> autodetection will be the primary use case, it already will not
>> work for every system (see EnterpriseUnix) and there must be some
>> way around it. This is that way.
> [SNIPPED]
> I don't believe this method needs to be public. It seems that the
> listFiles(FTPFileEntryParser parser, String pathname) is sufficient.
> A fully-qualified classname is required for the EnterpriseUnix use
> case anyway, right? And if you want to use autodetection, just use
> the listFiles(String pathname) version.
I meant to say that createFiles(String pathname, FTPFileEntryParser parser) is sufficient for this case.
> --
> =====================================================================
> Jeffrey D. Brekke jbrekke@wi.rr.com Wisconsin, USA brekke@apache.org
> ekkerbj@yahoo.com
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
--
=====================================================================
Jeffrey D. Brekke jbrekke@wi.rr.com
Wisconsin, USA brekke@apache.org
ekkerbj@yahoo.com
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [net] checked in parser factory implementation
Posted by "Jeffrey D. Brekke" <jb...@wi.rr.com>.
>>>>> On Mon, 05 Jan 2004 12:07:28 -0600, steve cohen <sc...@javactivity.org> said:
[SNIPPED]
> 4) rename getFileList(String parserKey, String pathname) to
> listFiles(String parserKey, String pathname). Even though
> autodetection will be the primary use case, it already will not work
> for every system (see EnterpriseUnix) and there must be some way
> around it. This is that way.
[SNIPPED]
I don't believe this method needs to be public. It seems that the
listFiles(FTPFileEntryParser parser, String pathname) is sufficient.
A fully-qualified classname is required for the EnterpriseUnix use
case anyway, right? And if you want to use autodetection, just use
the listFiles(String pathname) version.
--
=====================================================================
Jeffrey D. Brekke jbrekke@wi.rr.com
Wisconsin, USA brekke@apache.org
ekkerbj@yahoo.com
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [net] checked in parser factory implementation
Posted by steve cohen <sc...@javactivity.org>.
On Monday 05 January 2004 11:17 am, Daniel F. Savarese wrote:
>
> I disagree slightly (it's actually agreement, but with a naming
> difference) and have some suggested code changes (I can make them if
> there's agreement). First, the getFileList methods should be
> named listFiles() as that's what they're replacing. We can
> deprecate the old listFile methods that take FTPFileListParser
> arguments and reimplement listFiles() and listFiles(String pathname) in
> terms of the new stuff using autodection (removing
> getFileList(String parserKey) since the signatures conflict and the
> common case will be to use autodetection anyway). Second, we shouldn't
> create an FTPFileListParserFactory on every call to getFileList. Instead,
> FTPClient should maintain a reference to an FTPFileEntryParserFactory.
> This reference should be configurable by the user with a setter
> method. That allows API users to change the factory implementation.
> Third, we shouldn't set the default factory based on a property.
> Once we add the setter method, there is no need for the property.
> In general, it's better to let applications handle their configuration
> through properties and leave the library free of dependencies on property
> values. Which factory to use is an application-level decision (e.g.,
> an ant task can define a property that changes the factory for its
> purposes), not a library-level decision.
I have no disagreement with any of this. In fact the getFileList(String
parserKey) bothered me as much as it bothered you.
However, since I continue to have free time, I'd just as soon fix this stuff
myself. Just to verify that I have understood all your points:
1) deprecate listFiles() methods that take a FileEntryParser parameter.
2) reimplement listFiles() to do what getFileList(null) now does.
3) reimplement listFiles(String pathname) to do what getFileList(null,
pathname) now does.
4) rename getFileList(String parserKey, String pathname) to listFiles(String
parserKey, String pathname). Even though autodetection will be the primary
use case, it already will not work for every system (see EnterpriseUnix) and
there must be some way around it. This is that way.
5) don't use properties to select the parser factory in our library. Instead,
add a property and a setter method. Initialize in the library to use the
default. Have I missed anything?
One problem remains. getFileList() throws a new exception:
ParserInitializationException. If we simply rename these methods to
listFiles() we will break lots of code. That was my primary reason for giving
them a different name. One way around this would be to make
ParserInitializationException a RuntimeException. That makes sense, as the
error is not recoverable and is always due to a programming error - passing
in a key that the system cannot parse, specifying a class that has not been
made available. Would you agree?
(Parenthetical note: this distinction has always seemed a little confusing to
me and I think I finally understand why: the name. It should not have been
named "RuntimeException". All exceptions occur at runtime, both checked,
recoverable exceptions and those caused by programming errors. Joshua Bloch,
in Effective Java lays this out clearly enough. If it was called
PreconditionViolationException or some such, it would be much clearer. As it
is, I've seen too many instance of exceptions being made RuntimeExceptions
simply because the programmer thought that propagating it up though the call
stack was too difficult. But I don't think that's the case here. This is
legitimately a RuntimeException).
>
> Jeffrey Brekke writes:
> >list parsers, and/or implementing the existing list parsers with an
> >implementation that uses the entry parsers. That way we are parsing
> >with one implementation?
>
> That should be probably be our first step toward deprecation so the
> parsers behave consistently. In the 1.2 release we can deprecate the
> old listFiles methods presuming we agree to keep the method name for
> the files (the class method names currently mirror as closely as possible
> the FTP commands they implement). We're stuck by the Commons
> versioning/release rules regarding when we can remove the old stuff.
> It has to stay in all of the 1.x releases. Otherwise, I'd say that in
> 1.2 we announce that in 1.3 FTPFileListParserImpl will no longer
> implement FTPFileListParser and deprecate the interface in 1.2, removing
> it in 1.4 along with the old listFiles() methods. We'll just have to
> wait until 2.0. I just tested it, and if we deprecate FTPFileListParser
> now, it only affects the javadocs for FTPFileListParser. I think we
> can live with any compiler warnings saying that you're using a
> deprecated interface as long as we document this. With a J2SE 1.4
> compile, you only get warnings for the listFiles methods,
> DefaultFTPFileLister (which we also need to deprecate), and
> FTPFileListParserImpl, but none of the FTPFileListParserImpl subclasses.
> So I propose deprecation in 1.2 and removal in 2.0.
Agree here too, although the idea that FTPFileListParserImpl will no longer
implement FTPFileListParser is extremely odd. I suggest we might want rename
this class now to FTPFileEntryParserImpl. As it's abstract, I don't think
we'd be impacting anyone.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org