search

duncan-r / SHIP

Simple Hydraulic model Interface for Python
MIT License
7 stars 6 forks source link

Cross platform file path handling #6

Open JamesRamm opened 7 years ago

JamesRamm commented 7 years ago

Following on from the discussion from #4:

It appears as though we need changes to filetools.py and the unit tests in order to properly support cross platform handling of the file paths.

JamesRamm commented 7 years ago

My preferred solution is to use std lib functions where possible and avoid manual string parsing / separator replacement. This will mean that SHIP expects the input control files to be written for the OS ship is running on. I.e if SHIP is running on linux and input paths are windows style, errors would be thrown. I think this is a reasonable assumption as it will make development easier and allow for file validation (do all the inputs exist). However, I'm not really a tuflow user so happy to be argued down!

JamesRamm commented 7 years ago

If we do want to consider manipulating windows paths on *nix or vice versa, and/or use paths without having to know whether they actually exist, you could also consider using pathlib. This would introduce a dependency for python < 3.4 but is a std. lib. module after that: https://docs.python.org/3/library/pathlib.html#module-pathlib

duncan-r commented 7 years ago

I'll just cover some of the points you made in #4 and above. 1)

think it would be safe to assume that the machine SHIP is being run on is the same machine that tuflow would be run on?

I think you're right. However, we sometimes use it on models brought in from external clients to get them into a setup we like to use, for example. It could be that this comes in with unix paths and it would be useful to have a way to convert them over to run on our windows env. So it's less to do with the models we build than how we handle those that others have built.

2)

Then we can also easily write in validation checks for the file being read in (i.e. do those files actually exist).

The validation for existing files is definitely important. At the moment it's handled by either the TuflowModel (for the whole model) or the ControlFile (for specific files) using checkPathsExist(). This is provided as a public method, rather than done automatically when loading because there are times where we don't necessarily need to know whether a .shp/.csv/etc file exists (but sometimes we do and that's when the method is called). Although the load will fail when the .tgc/.ecf/.tbc/etc don't exist as obviously the model can't be read.

3)

A problem then for generating files and then passing the relative path is that it needs to be relative to something.

Absolutely. SHIP takes the .tcf directory and applies it as the root to all of the PathHolder instances to provide this. The updateRoot() method in TuflowModel just hands this down to all of them when a new model location is required. We use this for exporting models (with only the files associated with that model) out for delivery for instance. Although I see your point that it makes life difficult when converting between formats.

I think that everything you're saying is completely true. I don't want to get anywhere near trying to think of all the problems with manual path manipulation really. I also think that 99% of use cases will be as you said, running the tool and tuflow on the same machine. Perhaps this is a case for staying with the fix up the cross platform compatibility and once everything is a bit more solid some separate functionality can be built in to do os-to-os file conversations. A.k.a leave it for now? Or people can just run tools that deal with this sort of thing if they need?

JamesRamm commented 7 years ago

I think you're right. However, we sometimes use it on models brought in from external clients to get them into a setup we like to use, for example. It could be that this comes in with unix paths and it would be useful to have a way to convert them over to run on our windows env. So it's less to do with the models we build than how we handle those that others have built.

That is an interesting use case and one I certainly wouldn't have thought of, but makes complete sense! I think that as you say here:

once everything is a bit more solid some separate functionality can be built in to do os-to-os file conversations. A.k.a leave it for now?

Yes - this is a separate issue to be tackled once we are handling paths in general agnostically. An eventually nice interface would probably be something like a flag on loadFile/PathHandler's constructor to specify replacing path separators, and get it 'cleansed' before any validation or further processing occurs.

duncan-r commented 7 years ago

It's agreed then. Shelve for now.