Repository navigation
Conversation
|
I haven't done a full review yet, but note that a breaking API change like this will require a version bump to |
45d618d to
e3d5781
Compare
mr-smidge
left a comment
mr-smidge
left a comment
There was a problem hiding this comment.
Just left a couple of questions...
| namespace djinterop::util | ||
| { | ||
| /// Converts a UTF-8 encoded path string into a `std::filesystem::path`. | ||
| /// Returns a path as a UTF-8 encoded string, on all platforms. |
There was a problem hiding this comment.
Recommend being very explicit about the return type to avoid any confusion.
e.g. "Returns a path as UTF-8 encoded data in a std::string (note: not std::u8string), on all platforms."
There was a problem hiding this comment.
✅ Done in 65ef741, the doc now says it is a std::string (not std::u8string).
| boost::filesystem::path temp_dir{ | ||
| boost::filesystem::temp_directory_path()}; |
There was a problem hiding this comment.
Just wondering... do we actually need boost::filesystem at all now, or can it be removed as a dependency entirely?
There was a problem hiding this comment.
std has evolved a lot, would be nice to get rid of boost::filesystem. IMO not in this PR, but I'll investigate and file a PR if it's straightforward enough!
Functions that create, load or test for an Engine library now take the directory as a std::filesystem::path, and database::directory() returns one. Callers no longer need to know which encoding a path string is in. This is a breaking change: there are no std::string overloads left beside them. Track paths stored inside the database are strings, not filesystem paths, and stay std::string.
The directory arguments and return values in the public API are now std::filesystem::path, which breaks source compatibility.
OneLibrary support landed on main with std::string paths, so its database_impl::directory() no longer matched the base class and the build broke once this branch was rebased. Its functions that load, create or test for a database now take a std::filesystem::path, and directory() returns one, like the Engine functions.
7b82b2e to
4980e21
Compare
|
I've rebased onto main. OneLibrary support landed with I've also clarified the |
|
✅ Bumped to 0.28.0 in 99bf25f. |
As discussed in #205: the public API takes and returns directories as
std::filesystem::pathinstead ofstd::string.This is stacked on #205, so until that is merged the first two commits here are #205's. The change itself is the last commit, 45d618d.
What changes
create_database(),create_database_from_scripts(),create_or_load_database(),database_exists()andload_database()takeconst std::filesystem::path&.database::directory()andbase_engine_library::directory()returnstd::filesystem::path.v2::engine_libraryandv3::engine_library:create(),exists()andload()takeconst std::filesystem::path&./, and a smallpath_to_utf8()helper turns them into the UTF-8 strings SQLite expects.path_from_utf8()from Treat path strings as UTF-8 on Windows #205 is gone again.Track paths stored inside the database are strings, not filesystem paths, so they stay
std::string.For downstream users
This is a clean break: there are no
std::stringoverloads left, since they would make every call with a string literal ambiguous. Passing astd::stringstill compiles, through the implicit conversion topath. On Windows, though, a narrow string is read in the ANSI code page, so callers holding UTF-8 should construct the path from astd::u8string(or, with Qt, fromQString::toStdU16String()). Code that useddirectory()as a string needs.u8string(), or.string()where the ANSI code page is fine.The test suite passes (14/14 on Linux). The UTF-8 directory test from #205 now builds its path from a
u8""literal.