thanks for the feedback, Alan
based on suggestions, all issues have been addressed and patch has been
updated
http://cr.openjdk.java.net/~msheppar/8134577/webrev.07
the following should be noted:
ExtensionsWithLDAP.java is added to the exclude list with JDK-8134577 -
This is on the problem list as it will need to be re-written
The NameService associated with this test threw UnknownHostExceptions
and added the host name to a List, which was then examined
later in the test as a verification.
The instantiation of a NameService is as intended, i.e.
if jdk.net.hosts.file set and file exist then
instantiate HostsFileNameService
else
instantiate PlatformNameService
this is in keeping with previous initialization semantics - if the no
service provider NameService
created then the default NameService was instantiated.
regards
Mark
On 05/01/2016 13:10, Alan Bateman wrote:
On 05/01/2016 00:54, Mark Sheppard wrote:
Hi,
as per feedback below webrev has been updated
http://cr.openjdk.java.net/~msheppar/8134577/webrev.06/
change summary:
* List<NameService> nameServices replaced with Nameservice nameService
* references to nameServices removed
* private interface NameService added, with two implementation
PlatformNameService and HostsFileNameService
* Scanner created with UTF-8 charset
* removed StringTokenizer, replaced with String.split()
* comment handling extended, handling comments on line with mapping entry
* try with resources added to addMappingToHostsFile method in tests
I think this is starting to look good.
A few small comments:
- instantiateNameService (would createNameService be nicer?) still
falls back to PlatformNameService when the hosts file doesn't exist.
Is that expected?
- HostsFileNameService.hostsFile can be final.
- in removeComments then I assume you can just use indexOf and check
for -1 rather than contains + indexOf.
- it would be nice if NameService had some javadoc. Also the methods
don't need to be declared public.
- it would be nice if PlatformnameService and HostsFileNameService
could use /** */ style javadoc rather than //, just to keep the code
consistent.
- ExtensionsWithLDAP.java is added to the exclude list with
JDK-8134577, is there is separate issue for this?
- formatting. There are a few places where the indention is messed up
(5 spaces instead of 4 in a few places). Also some of the throws XXX
are intended in many different ways. Blank lines can help readability
but seem to be inconsistently used here. There's clearly been a lot of
revisions on this issue and maybe the simplest is to just reformat
this section of the file in the IDE.
-Alan.
