Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use os.path.join for file path handling #53

Open
wants to merge 2 commits into
base: experimental
Choose a base branch
from

Conversation

thomasZen
Copy link

(This is potentially shooting over the top as I changed more lines than I expected)

This PR simplifies the code by specifying the initial file paths directly as absolute paths and by consistently using os.path.join:

  • In QDSpy_global get the path to the QDSpy directory and set all following paths relative to that (e.g. using os.path.join(QDSpy_path, "Sounds")). This also gets rid of path concatenations and does not rely on fixing the paths later when running on Linux.
  • Use os.path.join for all subsequent path concatenation to stay platform independant.

This behavior changes the way people have to use the config in case they want to overwrite the defaults. They now have to specify absolute paths (potential downside), but these paths do not have to point to the same directory where QDSpy is (downside). If this is too big of a change I can either:

  • Change the PR to only fix the initial issue stated below
  • Put additional logic in QDSpy_config to expand the paths provided in the config if the directory they are pointing to does not exist

Initial issue

The following code changes the path that included the Stimulus directory (e.g. ./Stimulus/foo...) to point to the current directory if you are running on Linux. For me that was a problem, as the file path was off then and it did not load the file (similar for QDSpy_stim_video). This should be fixed with the changes above and worked on my Linux computer.

QDSpy/QDSpy_stim_movie.py

Lines 169 to 172 in 1505933

else:
tempDir = os.getcwd()
self.fNameDesc = fsu.repairPath(tempDir + tempStr) + glo.QDSpy_movDescFileExt
self.fNameImg = fsu.repairPath(tempDir + tempStr) + self.fExtImg

@@ -72,7 +75,7 @@
QDSpy_cPickleFileExt = ".pickle"
QDSpy_fileVersionID = 8
QDSpy_stimFileExt = ".py"
QDSpy_pathStimuli = ".\\Stimuli\\"
QDSpy_pathStimuli = os.path.join(QDSpy_path, "Stimuli") + os.path.sep

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Very minor point] If you are switching to os.path.join everywhere, I think you can drop adding the os.path.sep at the end, as it will be added any time you join a path onto this path.

Also, if you are just starting on the task of reworking path management, I'd choose pathlib over os.path, as the former is easier to work with. Just my opinion though.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are switching to os.path.join everywhere, I think you can drop adding the os.path.sep at the end, as it will be added any time you join a path onto this path.

I was not sure if at any points in the code that I missed we still use string concatenation (I searched for \\ in the code and adapted all those occurrences, but there might be some I missed that make the asusmption that every path in the global.py file ends with a path separator.

Also, if you are just starting on the task of reworking path management, I'd choose pathlib over os.path, as the former is easier to work with. Just my opinion though.

I only adjusted the code so far that my initial use case worked on Linux, which lead to the error in the PR description, and spend some effort to make the code consistent wrt to path concatenation. I would leave it at that, though. Also if you'd want to spend more time on it, feel free to directly adjust this PR to use pathlib.

Did you happen to use this branch? If so did it work, and did you use it on Windows or Linux?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dear both,

sorry for replying so late.

Path management can be really annoying and I agree with the need of a better solution.
However, I am not too happy with the solution you suggested @thomasZen because I expect side effects on the Windows users. I haven't tested the changes yet, but instead suggest a different procedure:

(1) The reason I introduced QDSpy_file_support.py was to have all path/file related functions in one place, where one can take care of OS-specific differences. Therefore, I suggest to change the functions there instead and adding support functions if needed, instead on patching the code. E.g., instead of adding os.path.join() using a function in QDSpy_file_support.py, where we can implement the function either with pathlib or os.path. (see below) This way, we prevent mixing the libraries and stay flexible.

(2) I agree with @kevindoran in that pathlib is likely the better choice. I used it for a different project and had surprisingly free problems with paths on three different systems (Windows, Linux, and a proprietary DOS-like OS).

I can try to make a suggestion based on experimental and your changes @thomasZen and then you can test, o.k.?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants