GandaG / pyfomod

A high-level fomod library written in Python.
https://pyfomod.rtfd.io/
Apache License 2.0
7 stars 6 forks source link

TypeError when passing a string to path parameter on Installer #9

Closed Voxed closed 4 years ago

Voxed commented 4 years ago

Installer.files() returns a dictionary of of destination/source key-pairs, this makes it impossible to have multiple options copy files to the same directory seeing as there can only be one key-pair per destination.

A fix could be using a list of tuples instead.

GandaG commented 4 years ago

Installer.files() should return a mapping of source -> destination - but I'm not seeing the use case for this. The entire point of that was to do the priority "math" for the end-user so they wouldn't need to worry about them.

The only point I can think of would be in targeting the same folder but pyfomod handles only files.

Voxed commented 4 years ago

I see, I was under the impression that the Installer was supposed to function with any fomod file. Copying entire directories is a common practice. This might have been out of line.

GandaG commented 4 years ago

the Installer was supposed to function with any fomod file

It does, but I realize that my statement above may be misinterpreted - I meant the files() method should only return a map of file source -> file dest, such that any folder sources have been "broken down" into the files within. See here. The point is so that users don't have to worry about what has priority over what and whether something is a folder or not, pyfomod handles all that and returns only files.

So if you specify a example_dir/ folder source that contains example_dir/example1.png and example_dir/example2.txt to a boop destination you should get a map like:

{
  "example_dir/example1.png": "boop/example1.png",
  "example_dir/example2.txt": "boop/example2.txt"
}
GandaG commented 4 years ago

Did you actually find a bug or was this from reading the docs?

Voxed commented 4 years ago

I see, I was not having this outcome when testing the library so I assumed it didn't do this. I will further investigate what could have caused it to fail earlier.

GandaG commented 4 years ago

Sure, leave some feedback with an example!

Voxed commented 4 years ago

This was an error on my part, I was under the impression that the path argument passed to the Installer was supposed to be the output directory and not the fomod root directory.

I was running into the error TypeError: unsupported operand type(s) for /: 'str' and 'str' when only providing the root path to the installer, not knowing it needed to be of type Path and didn't think it was logical to have to provide the root path twice.

Might I suggest changing this to

if isinstance(root, Path) and path is None:
    self.path = root
elif isinstance(root, str) and path is None:
    self.path = Path(root)

in order to allow for string root paths.

GandaG commented 4 years ago

Ah, finally, a bug! :D Fixing it right now.

GandaG commented 4 years ago

Fixed in 3f6d6b58daac1864d31594bc12284100fa7a8903