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

std::filesystem::path support #34

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

Conversation

MeGaL0DoN
Copy link

I kept the original constructors accepting std::string, but created a new class "mINIFilePath" with an explicit std::filesystem::path constructor. It is needed so any existing code is not broken, in case there is some type which implicitly converts to std::string but not to std::filesystem::path. So, for example, mINI::INIFile file { "hello.txt" } will still call the std::string version,
but, mINI::INIFile file { mINI::mINIFilePath("hello.txt") } will call the std::filesystem::path version. This is important because std::filesystem::path can be created with any encoding, allowing the library to be used to read/write files containing unicode characters.

@metayeti
Copy link
Owner

The idea is fine but I think it needs some changes still.

  1. Instead of a wrapper, wouldn't it be more on point to just allow mINI::INIFile(std::filesystem::path&) or such? Keep any paths internally with filesystem::path but just expose a string API for backwards compatibility. Basically a simpler implementation.
  2. There needs to be some sort of testcase added for this.

If you want I can figure this out myself but it will take me some time as I'm busy with other stuff at the moment.

@metayeti
Copy link
Owner

Actually doesn't the filesystem API by default allow strings anyway?

Copy link

@bilal614 bilal614 left a comment

Choose a reason for hiding this comment

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

Hi. I started using mINI recently in a repo of mine, really appreciated how easy mINI was to integrate and use on my project. Please feel free to ignore the review comment if it seemed totally out of the blue and unsolicited :) .

std::filesystem::path value;
explicit mINIFilePath(std::filesystem::path const& p) : value(p) {}

static mINIFilePath fromString(std::string const& str)

Choose a reason for hiding this comment

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

Was wondering if the fromString static method could be an overloaded constructor taking a string path instead? It may make the calling convention less verbose for example. For instance mINIFilePath(filename) instead of mINIFilePath::fromString(filename)

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah I'm gonna need to simplify this. I don't think there's a need for a wrapper.

Copy link
Author

@MeGaL0DoN MeGaL0DoN Jun 1, 2024

Choose a reason for hiding this comment

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

Yeah I'm gonna need to simplify this. I don't think there's a need for a wrapper.

Wrapper is needed if you want it to be 100% backwards compatible with all existing code. For example, if someone added conversion operator to std::string to some class:

struct test
{
    int val;

    operator std::string() const
    {
        return std::to_string(val);
    }
};

Then they won't be able to pass it to the function accepting std::filesystem::path. And you can't have function accepting both, std::string and std::filesystem::path directly either, because you will get compiler error when trying to pass a string directly, since string can be implicitly convert to filesysyem::path, and it won't know which overload to use. Try it yourself.

Copy link
Author

Choose a reason for hiding this comment

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

Was wondering if the fromString static method could be an overloaded constructor taking a string path instead? It may make the calling convention less verbose for example. For instance mINIFilePath(filename) instead of mINIFilePath::fromString(filename)

You can't have 1 constructor accepting std::string and the other std::Filesystem::path, since string by itself is implicitly convertible to std::filesystem::path, so it won't know which overload to use, and it will result in a compile error.

Copy link
Owner

@metayeti metayeti Jun 2, 2024

Choose a reason for hiding this comment

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

Thanks for insights, I'll experiment with it when I have some time on my hands.

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