Improve and test Path implementation #21

New Issue

FWDekker · 2019-10-31T10:33:57+01:00

FWDekker commented

2019-10-31 10:33:57 +01:00

The implementation of Path should be tested. A testing framework has been set up but it is not currently being used.

Testing the Path interface will probably reveal some flaws in the usage of Path. I believe the way it is currently used is suboptimal and there should be other ways to use it. For example, it should not be necessary to call new Path(path).path to optimise a path, and it should be possible to call getNode(path: string): string on a Directory with path set to dir1/dir2/file to simplify file retrieval. (Though this will require path optimisation before calling this method since a directory does not know its parent and can therefore not retrieve ../file.)

Changing the FileSystem's _cwd field to a Path should also be considered, though I'm not sure this is feasible/useful/better.

Finally, as is already listed in some TODOs, the fields of Path should be turned into getters.

The implementation of `Path` should be tested. A testing framework has been set up but it is not currently being used. Testing the `Path` interface will probably reveal some flaws in the usage of `Path`. I believe the way it is currently used is suboptimal and there should be other ways to use it. For example, it should not be necessary to call `new Path(path).path` to optimise a path, and it should be possible to call `getNode(path: string): string` on a `Directory` with `path` set to `dir1/dir2/file` to simplify file retrieval. (Though this will require path optimisation before calling this method since a directory does not know its parent and can therefore not retrieve `../file`.) Changing the `FileSystem`'s `_cwd` field to a `Path` should also be considered, though I'm not sure this is feasible/useful/better. Finally, as is already listed in some `TODO`s, the fields of `Path` should be turned into getters.

FWDekker self-assigned this 2019-10-31 10:33:57 +01:00

FWDekker added the

code-quality

label 2019-10-31 10:33:57 +01:00

FWDekker commented

2019-10-31 14:08:06 +01:00

Issues with Path's implementation become especially apparent when looking at the code for cp and mv in fs.ts.

Issues with `Path`'s implementation become especially apparent when looking at the code for `cp` and `mv` in `fs.ts`.

FWDekker added the

bug

label 2019-10-31 14:19:19 +01:00

FWDekker commented

2019-10-31 14:22:42 +01:00

Funky things start happening. Consider the following undesirable behaviours:

Running cp -r . . in root will create a subdirectory / in root. This should not be possible.
Running mkdir / in root will create a subdirectory / in root. This should not be possible.
Running mkdir . in root will create a subdirectory / in root. This should not be possible.
Running cd personal and then cp ../resume.pdf ./ will give the error The file resume.pdf already exists. This is incorrect.

Funky things start happening. Consider the following undesirable behaviours: 1. Running `cp -r . .` in root will create a subdirectory `/` in root. This should not be possible. 2. Running `mkdir /` in root will create a subdirectory `/` in root. This should not be possible. 3. Running `mkdir .` in root will create a subdirectory `/` in root. This should not be possible. 4. Running `cd personal` and then `cp ../resume.pdf ./` will give the error `The file resume.pdf already exists`. This is incorrect.

FWDekker referenced this issue from a commit

2019-10-31 22:18:09 +01:00

Fix #21

FWDekker closed this issue

2019-10-31 22:18:09 +01:00

Sign in to join this conversation.