Pull requests

#4 Declined
Repository
Deleted repository
Branch
default (35cc98df21d3)
Repository
Gfy/pyReScene pyReScene
Branch
default

Fixes for the tests on Linux

Author
  1. vadmium
Reviewers
Description

These changes get the test suite working under Linux. But before you pull these changes, perhaps you could consider what path format the “RarBlock.file_name” fields should use in the “rescene.rar” module, after reading them from an SRR file. Should they use the OS’s preferred path separator, or always use a forward slash (/)?

Currently I think the “rescene.rar” module reads the file name field from the SRR blocks verbatim, with no conversion. This means it usually has forward slashes (/), but may occasionally have backward slashes (\), which are not interpreted as directory separators on Linux. In my revision c8a8ed1, “Convert internal SRR paths to OS-specific form”, I made it always convert to the preferred OS path format.

But it turned out this broke some of the tests under Windows, because they were actually expecting a forward slash. You can see in that revision I had to change some of the tests, for example to use os.path.join() rather than a hard-coded forward slash. If you preferred, I could amend this revision to always convert to forward slashes instead, which might reduce the chance of interfering with existing usage. What do you think?

Related: my revision 67879b8, “Convert Rar packed file names to OS-specific paths”, though this didn’t cause any problems on Windows because only backslashes are used, before and after.

Other than the path conversion issue, there are some other fixes for the unit tests here, and a couple cleanups and initial Python 3 porting (byte string literals) that helped with character encoding issues in the unit tests. The rest of my Python 3 port will go on top of these changes. Also, I added a file “builtintests.py” which runs all the tests by name that I could find, since test discovery isn’t supported on Python 2.6, and I don’t have the Nose package installed.

  • Learn about pull requests

Comments (3)

  1. Gfy repo owner

    The paths of the archived files stored to the SRR file must use / internally. [1] Same for the names of RAR files. [2] (should also not start with a /)

    On srrdb the / separator must be used when adding paths to file names and is also always shown. I don't think there are real SRR files that use a backslash though, but if it ever occurs, pyReScene should be able to handle those.

    On the output of srr -l file.srr I'd like to see what is actually stored in the SRR. (So almost always it should show /) And especially with the -e parameter. It should be similar to urls: you only see forward slashes. (but I want to be able to see it if something is starting to behave weird)

    So, these are the requirements :) I don't have a preference of how exactly it should be accomplished.

    1. vadmium author

      Okay, neither of my propositions are good then; both would show forward slashes in the list output, even when they are really backslashes. I think what we really need is another attribute, maybe “os_file_name” or something, or a function or method to convert the internal SRR stored file names and Rar file names to OS-specific format on demand. I’ll have a think about it and try to update this pull request when I get a chance.

      For the record, the relevant test is rescene.test.test_main.TestExtract.test_extract_srr_path_backslash(), https://bitbucket.org/Gfy/pyrescene/src/fa5bf98f779d7c7db79ef4c44c0353891ce77f3d/rescene/test/test_main.py#cl-111. It extracts from “test_files/store_little/store_little_srrfile_with_path_backslash.srr”, which contains the stored file name “store_little\store_little.srr”, with a backslash.

  2. vadmium author

    Ergh. If you are able to, please close this pull request and see pull request #5 instead.

    I ran into some issues with Bitbucket not letting me update this pull request. Then it wouldn’t let me strip changes from my repository. So I tried deleting and re-forking my repository, and now I’ve even lost the “Decline” button at the top :(