search

OutpostUniverse / OP2Utility

C++ library for working with Outpost 2 related files and tasks.
MIT License
4 stars 0 forks source link

Fix XFile::RemoveFilename so it does not return the filename when no … #231

Closed Brett208 closed 5 years ago

Brett208 commented 5 years ago

…directory is passed

Also add unit tests for RemoveFilename

Brett208 commented 5 years ago

It looks like Linux and Windows return different results. We could tailor the tests to be if Windows and if Linux?

The point of this branch is that RemoveFilename would return Ashes.map when provided Ashes.map. I want it to return "." or "./" or "". That way I'm not using a filename when attempting to use a directory.

So yeah, this one may be contentious and difficult to deal with. I don't think we will be able to get OP2MapImager to work reliably without addressing though. I'd like to get a better release of OP2MapImager out in the near future.

-Brett

DanRStevens commented 5 years ago

This looks like the same issue I ran into in #216. It was solved in https://github.com/OutpostUniverse/OP2Utility/pull/216/commits/214a657acd174e8edfa6d7a5803ef5fb102099ff by using .generic_string() instead of .string().

Side note: The name RemoveFilename sounds kind of like it deletes a file, rather than returns the directory portion of the path. This seems to be a case of naming something after how it was implemented, rather than what it does (which is why the name "BitTwiddle" I recently chose sucked).

I would recommend renaming this method to something like Directory, or GetDirectory. I would also recommend using the platform independent path separators by default, and to include trailing slashes.

DanRStevens commented 5 years ago

Interesting. It seems the platforms have an inconsistency regarding the trailing slash.

DanRStevens commented 5 years ago

I'm just realizing we have some duplication here. What's the difference between GetDirectory and RemoveFilename? They appear to both do the same thing.

Brett208 commented 5 years ago

Now that you mention it, both functions accomplish the same thing. I'm okay deleting one. Do you prefer one name over the other? I would probably prefer just keeping RemoveFilename, but not a big deal either way.

We may want to consider forcing a trailing slash in both builds so we have consistent results.

Thanks, Brett

DanRStevens commented 5 years ago

Agreed about forcing a trailing slash.

I think the current implementation of GetDirectory already forces a trailing slash, based on the test code already added and passing in the other pull request.

For the name, I would prefer going with GetDirectory, or maybe rename to GetDirName. The name RemoveFilename sounds very misleading to me.