JSR 203 file system implementation

[Calsir]

@uckelman

(Sure, remark away.) Why do you say that?

Because “l” and “1” are too similar. Of course, they are not the same as the fonts I use discriminate between them, but they can get confusing in already packed methods.

[uckelman]

Thus spake “Calsir”:

I fixed findRootSep and solved this issue: string.matches matches the whole s
tring to the regex, rather than only the beginning.

I used File.listRoots() to get the available roots in WindowsPath, UNC names
are still resolved through regexes.

I’m looking at what you did with WindowsPath.findRootSep() and I think it’s
too restrictive. There’s nothing in the Path docs which indicates that a Path
has to exist on the system where you’re running in order for it to be
absolute. But “Q:\foo\bar” is an absolute path (on a Windows box) irrespective
of whether there is any drive Q.

Hence, I think it should be put back the way it was. Comments?

I am still trying to figure out how to do normalize() properly. The current i
mplementation fails to consider multiple .. occurrences, as in c:\dir1\dir2
…......\dir3 (which should target c:\dir3).

Hmm. I’ll take a look at that.

–
J.

---

Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

[uckelman]

Thus spake “Calsir”:

uckelman wrote:

(Sure, remark away.) Why do you say that?

Because “l” and “1” are too similar. Of course, they are not the same as the
fonts I use discriminate between them, but they can get confusing in already
packed methods.

That’s what I thought your reason would be. You’ve already anticipated my
reply, too, though I would say it a bit differently: Fonts where 1 and l
look the same should not be used for programming.

I’ll change it to ‘p’, which is more evocative of what it’s for. (l stood
for ‘List’, which also pretty crappy.

Thank you for keeping me honest.

–
J.

---

Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

[uckelman]

Thus spake “Calsir”:

uckelman wrote:

Absolutely. But Java doesn’t permit abstract static methods.

I should have seen that coming…

I thought originally that there would be a lot of difference between
the Windows and UNIX classes, but it’s looking now like the sole point
of difference is what we do in findRootSep(). I’m going to collapse all
of the Windows and UNIX classes back into the Real classes. That should
simplify things somewhat.

–
J.

---

Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

[Calsir]

@uckelman

But “Q:\foo\bar” is an absolute path (on a Windows box) irrespective
of whether there is any drive Q.

Hence, I think it should be put back the way it was. Comments?

Good point. I will revert the code to use regex. A non-existent path is a path nonetheless. Existence is not a perfection .

Hmm. I’ll take a look at that.

I was thinking on some logic like this:

use the parts separators to create a list of strings (elements).
remove occurrences of “.” (current directory).
count the number of occurrences of “…” and their last position.
keep every element to the right of the last position of “…” .
for every “…”, remove an element, from right to left, to the left of the last position of “…”.
if there are remaining “…” and the path is absolute, delete them, otherwise, keep them.
rebuild the path string.

I was writing some code tonight but it got quite messy. I will look into it tomorrow.

[uckelman]

Thus spake “Calsir”:

I am still trying to figure out how to do normalize() properly. The current i
mplementation fails to consider multiple .. occurrences, as in c:\dir1\dir2
…......\dir3 (which should target c:\dir3).

I believe that the normalize() I just committed works. (It passes the
test, at least.) I switched from working from the end to working from
the beginning.

I just did some major refactoring:

• Since the only difference between Windows and Unix is the implementation
  of findRootSep(), I collapsed UnixFileSystem and WindowsFileSystem into
  RealFileSystem, and UnixFileSytemProvider and WindowsFileSystemProvider
  into RealFileSystemProvider.

• RealFileSystem now creates RealPaths using a RealPathFactory. The
  RealPathFactory instance creates either RealUnixPaths or RealWindowsPaths,
  which subclass RealPath.

• I also added JUnit assumeTrue()s to stop tests which cannot succeed on
  the platform where their being run. What I’d like to do is make as many
  of the test platform-agnostic as possible, which I think we can do by
  fixing up the strings we get from File when we’re not on the right platform.
  I’m going to have weirdly fragmented day tomorrow, so I’ll probably have
  some time to mess with this particular point.

• If you don’t like what I’m doing, complain. It’s been a while since
  I’ve been working on the same code with someone else like this. Brent,
  Michael, and I have generallly been working separately on different parts
  of the codebase.

–
J.

---

Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

[uckelman]

Thus spake “Calsir”:

uckelman wrote:

But “Q:\foo\bar” is an absolute path (on a Windows box) irrespective
of whether there is any drive Q.

Hence, I think it should be put back the way it was. Comments?

Good point. I will revert the code to use regex. A non-existent path is a pat
h nonetheless. Existence is not a perfection .

Already done it. (Otherwise we’d have to contend with the best of all
possible paths existing on every machine…)

Hmm. I’ll take a look at that.

I was thinking on some logic like this:

use the parts separators to create a list of strings (elements).
remove occurrences of “.” (current directory).
count the number of occurrences of “…” and their last position.
keep every element to the right of the last position of “…” .
for every “…”, remove an element, from right to left, to the left of the las
t position of “…”.
if there are remaining “…” and the path is absolute, delete them, otherwise,
keep them.
rebuild the path string.

I was writing some code tonight but it got quite messy. I will look into it t
omorrow.

Also already done. I’m working from the left. Whenever I see a “…”, I
chuck the previous name, unless there isn’t one. That seems to work.

–
J.

---

Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

[Calsir]

@uckelman

I believe that the normalize() I just committed works. (It passes the
test, at least.) I switched from working from the end to working from
the beginning.

From what I read on the next post, it should, and it is simpler than mine.

• RealFileSystem now creates RealPaths using a RealPathFactory. The
  RealPathFactory instance creates either RealUnixPaths or RealWindowsPaths,
  which subclass RealPath.

I haven’t got a bloody clue on what a factory is, only that factories exist. I guess I’ll take a look tomorrow .

• I also added JUnit assumeTrue()s to stop tests which cannot succeed on
  the platform where their being run. What I’d like to do is make as many
  of the test platform-agnostic as possible, which I think we can do by
  fixing up the strings we get from File when we’re not on the right platform.
  I’m going to have weirdly fragmented day tomorrow, so I’ll probably have
  some time to mess with this particular point.

I was working on that on the later tests: trying to use Paths.get() to create the paths. I was thinking of migrating most of the methods from WindowsPathTest to RealPathTest. Once this is done, Unix* should not pose too much of a problem.

• If you don’t like what I’m doing, complain. It’s been a while since
  I’ve been working on the same code with someone else like this. Brent,
  Michael, and I have generallly been working separately on different parts
  of the codebase.

Let me handle the tests, as my specific knowledge of java is quite lacking: it is easier to write the tests and reading the code than writing the actual production code, as the latter requires experience and general knowledge of the “trade”.

Already done it. (Otherwise we’d have to contend with the best of all
possible paths existing on every machine…)

Nothing to add.

[uckelman]

Thus spake “Calsir”:

• RealFileSystem now creates RealPaths using a RealPathFactory. The
  RealPathFactory instance creates either RealUnixPaths or RealWindowsPaths,
  which subclass RealPath.

I haven’t got a bloody clue on what a factory is, only that factories exist.
I guess I’ll take a look tomorrow .

A factory is just an interface that produces something else, where the
particular something else is specified by concrete instances of the factory.

en.wikipedia.org/wiki/Factory_object

Specifically, what I did was follow the Abstract Factory Pattern:

en.wikipedia.org/wiki/Abstract_factory_pattern

–
J.

---

Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

[uckelman]

Thus spake Joel Uckelman:

A factory is just an interface that produces something else, where the
particular something else is specified by concrete instances of the factory.

en.wikipedia.org/wiki/Factory_object

Specifically, what I did was follow the Abstract Factory Pattern:

en.wikipedia.org/wiki/Abstract_factory_pattern

In fact, if you look at the Java example here, the first interface and
two classes (GUIFactory, WinFactory, and OSXFactory) are exactly
analogous to what I did.

–
J.

---

Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

[uckelman]

Thus spake “Calsir”:

I was working on that on the later tests: trying to use Paths.get() to create
the paths. I was thinking of migrating most of the methods from WindowsPathT
est to RealPathTest. Once this is done, Unix* should not pose too much of a p
roblem.

Sounds good. This should eliminate most of the duplicate code.

• If you don’t like what I’m doing, complain. It’s been a while since
  I’ve been working on the same code with someone else like this. Brent,
  Michael, and I have generallly been working separately on different parts
  of the codebase.

Let me handle the tests, as my specific knowledge of java is quite lacking: i
t is easier to write the tests and reading the code than writing the actual p
roduction code, as the latter requires experience and general knowledge of th
e “trade”.

Ok. I’ll focus on passing the tests, then.

–
J.

---

Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

[uckelman]

Thus spake “Calsir”:

Let me handle the tests, as my specific knowledge of java is quite lacking: i
t is easier to write the tests and reading the code than writing the actual p
roduction code, as the latter requires experience and general knowledge of th
e “trade”.

BTW, I noticed that we have a few tests right now which contain multiple
assertions. It would be nicer to have only one assertion per test. See
question #10:

junit.sourceforge.net/doc/faq/faq.htm#tests_10

–
J.

---

Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

[Calsir]

@uckelman

BTW, I noticed that we have a few tests right now which contain multiple
assertions. It would be nicer to have only one assertion per test. See
question #10:

junit.sourceforge.net/doc/faq/faq.htm#tests_10

Got it. Will comply. I am trying to refactor most of the methods of RealWindowsPathTest into a new RealPathTest.

Clever implementation of normalize() as it does not involve too deep a research or multiple lists (as mine did).

[Calsir]

I did some refactoring: RealWindowsPathTest and RealUnixPathTest now declare 2 tests only, the rest is inherited from RealPathTest.

Just one quirk. According to JUnit javadoc, any @Before method in a parent class should be executed first. However, if I don’t call a super.setUp() in Real(Windows|Unix)PathTest.setUp(), all tests fail with NullPointerException.

I will now work on splitting the test into single asserts. I will just keep multiple asserts where an exception is not expected in the test (with File operations), rather than use assumeTrue().

[Calsir]

There are still some errors in the logic of splitPath() and normalize(). I think that the issue lies in the handling of the indexes in substring() calls, due to the last element of parts being greater than the length of the path string. Wouldn’t rewriting RealPath to have the fields

ArrayListpathElements;
String root;

allow for simpler handling of every method? It would also allow much simpler code for creating new paths.

[uckelman]

Thus spake “Calsir”:

Just one quirk. According to JUnit javadoc, any @Before method in a parent cl
ass should be executed first. However, if I don’t call a super.setUp() in Rea
l(Windows|Unix)PathTest.setUp(), all tests fail with NullPointerException.

The reason for this is that if you override a method, the version from the
parent class won’t be called unless you explicitly call it. (Exception:
In constructors, super() will be called first unless you expilicitly call
a parent constructor with some other signature.)

–
J.

---

Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

[uckelman]

Thus spake “Calsir”:

There are still some errors in the logic of splitPath() and normalize(). I th
ink that the issue lies in the handling of the indexes in substring() calls,
due to the last element of parts being greater than the length of the path st
ring. Wouldn’t rewriting RealPath to have the fields

I’m working on fixing this right now…

ArrayListpathElements;
String root;

allow for simpler handling of every method? It would also allow much simpler
code for creating new paths.

Funny thing, I had a separate root component at one point switched it to the
way it is now for exactly the same reason—it’s simpler this way.

Actually, there is another problem with keeping the path elements as Strings:
In the event that you have a large number of RealPaths existing at once,
then you start to get a lot of overhead from all of the extra String objects.

–
J.

---

Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

[uckelman]

Thus spake “Calsir”:

There are still some errors in the logic of splitPath() and normalize(). I th
ink that the issue lies in the handling of the indexes in substring() calls,
due to the last element of parts being greater than the length of the path st
ring. Wouldn’t rewriting RealPath to have the fields

ArrayListpathElements;
String root;

allow for simpler handling of every method? It would also allow much simpler
code for creating new paths.

I switched the int to hold the positions of the separators, rather than
the positions of the beginnings of the names. This makes access more
uniform; I think it’s less consufusing this way.

Also, it works on the normalize tests (except for ‘…/…’, which is a case
I hadn’t counted on—leading ‘…’ cannot be eaten in a relative path).
Part of the problem was that the normalize tests were wrong—the Strings
being tested against ended with a separator (which the output can’t, ever),
and also we were testing whether a String equals a Path.

–
J.

---

Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)

[Calsir]

@uckelman

Thus spake “Calsir”:

I switched the int to hold the positions of the separators, rather than
the positions of the beginnings of the names. This makes access more
uniform; I think it’s less consufusing this way.

Good.

Also, it works on the normalize tests (except for ‘…/…’, which is a case
I hadn’t counted on—leading ‘…’ cannot be eaten in a relative path).

I fixed normalize() in my working copy to cope with that.

Part of the problem was that the normalize tests were wrong—the Strings
being tested against ended with a separator (which the output can’t, ever),
and also we were testing whether a String equals a Path.

Having seen your comment in the commit(), I checked both dos behaviour with multiple separators and with trailing separators, as well as their handling by the File class: they are simply ignored. Run this snippet to check how unix behaves:

String fName = File.listRoots()[0].toString() + "test" + File.separator + File.separator + "test2" + File.separator + "." + File.separator;
System.out.println(new File(fName).getPath());

In windows, it returns:
C:\test\test2.

I apologize for the mistake nonetheless. As for the other part, I never actually got to the test result.

[uckelman]

Thus spake “Calsir”:

Also, it works on the normalize tests (except for ‘…/…’, which is a case
I hadn’t counted on—leading ‘…’ cannot be eaten in a relative path).

I fixed normalize() in my working copy to cope with that.

Wanna commit that?

Part of the problem was that the normalize tests were wrong—the Strings
being tested against ended with a separator (which the output can’t, ever),
and also we were testing whether a String equals a Path.

Having seen your comment in the commit(), I checked both dos behaviour with m
ultiple separators and with trailing separators, as well as their handling by
the File class: they are simply ignored. Run this snippet to check how unix
behaves:

Code:

String fName = File.listRoots()[0].toString() + “test” + File.separator + Fil
e.separator + “test2” + File.separator + “.” + File.separator;
System.out.println(new File(fName).getPath());

In windows, it returns:
C:\test\test2.

In Linux, I get: /test/test2/.

That’s not a test of what I was talking about, though—File removes
duplicate and trailing separators, but the Strings we were comparing against
were created by hand, not by File.getPath().

I think the way to avoid this problem in future is either to:

1. define curDir and prevDir so that the separator is first: curDir =
   separator + “.” insetead of “.” + separator, or

2. build these strings using String.replace():

“thisDir/././dir2/./.”.replace(“/”, File.separator)

I find #2 a lot more readable than what we’re doing now, BTW.

I apologize for the mistake nonetheless. As for the other part, I never actua
lly got to the test result.

No problem. It’s solved. We learned something.

–
J.

---

Messages mailing list
Messages@forums.vassalengine.org
forums.vassalengine.org/mailman/ … engine.org

Post generated using Mail2Forum (mail2forum.com)