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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[馃殌 Feature]: Ability to overwrite files when using FileLogHandler #13860

Open
MJB222398 opened this issue Apr 23, 2024 · 6 comments 路 May be fixed by #13900
Open

[馃殌 Feature]: Ability to overwrite files when using FileLogHandler #13860

MJB222398 opened this issue Apr 23, 2024 · 6 comments 路 May be fixed by #13900
Labels
C-dotnet help wanted Issues looking for contributions I-enhancement

Comments

@MJB222398
Copy link

Feature and motivation

I want to make use of FileLogHandler to output driver logs to the filesystem for the testing suite (one file per test). Currently this only supports appending logs to the file. I do not need to keep logs from previous test runs and would prefer to create new files each time so that I only have the relevant logs.

Usage example

The solution could be as simple as having an overload constructor for the FileLogHandler:

public FileLogHandler(string filePath, bool overwriteExisting)

Copy link

@MJB222398, thank you for creating this issue. We will troubleshoot it as soon as we can.


Info for maintainers

Triage this issue by using labels.

If information is missing, add a helpful comment and then I-issue-template label.

If the issue is a question, add the I-question label.

If the issue is valid but there is no time to troubleshoot it, consider adding the help wanted label.

If the issue requires changes or fixes from an external project (e.g., ChromeDriver, GeckoDriver, MSEdgeDriver, W3C), add the applicable G-* label, and it will provide the correct link and auto-close the issue.

After troubleshooting the issue, please add the R-awaiting answer label.

Thank you!

Copy link

This issue is looking for contributors.

Please comment below or reach out to us through our IRC/Slack/Matrix channels if you are interested.

@nvborisenko
Copy link
Member

It is good idea.

I am curious whether reserving public FileLogHandler(string filePath, bool overwrite) method signature is a good choice. What if in the future we will need one more boolean flag? For instance deleteIfExists, or alwaysAppendNewLine. We can make this configuration as a property, but usually properties reflect a state of an object... bad choice.

I vote for optional argument:

public FileLogHandler(string filePath, bool overwrite = false)

In this case we should double-check whether introducing new optional argument is breaking change (it is not at compilation level, I am not sure about binary level).

@YevgeniyShunevych your input is highly appreciated.

@MJB222398
Copy link
Author

Or the constructor could take an "options" object that could be extended whenever more configuration is required and then that way the constructor signature won't change after this point.

@YevgeniyShunevych
Copy link
Contributor

Agree, that can be a useful ability.

Adding an extra optional parameter, such as FileLogHandler(string filePath, bool overwrite = false) is a breaking change at a binary level. But it can be avoided by overloading constructor, leaving old constructor FileLogHandler(string filePath) and adding a new one FileLogHandler(string filePath, bool overwrite).

But if we really expect some other parameters in the future, then options class looks a better way. But anyway I would add the second constructor, considering a binary breaking change.

public class FileLogHandler
{
    public FileLogHandler(string filePath)
        : this(filePath, FileLogHandlerOptions.Default)
    {
    }

    public FileLogHandler(string filePath, FileLogHandlerOptions options)
    {
        //...
    }
}

public sealed class FileLogHandlerOptions
{
    public static FileLogHandlerOptions Default { get; } = new FileLogHandlerOptions();

    public bool Override { get; init; }
}

@nvborisenko
Copy link
Member

Thank you guys!

In summary, we should choose between:

ctor FileLogHandler(string filePath);
ctor FileLogHandler(string filePath, bool overwrite = false);
ctor FileLogHandler(string filePath);
ctor FileLogHandler(string filePath, FileLogHandlerOptions options);

The second option dictates to apply this good pattern (how to avoid breaking changes) anywhere! I don't think it is good idea, at least this pattern is not spread widely in BCL. Our class is pretty simple, we don't expect new "flags" (at least how I feel it). But the second option is CLS-compliant (lol, should we).

In any case user is able to implement his own ILogHandler. I prefer to be as simpler as possible here.

Just to confirm, is the following is a breaking change?
Before:

ctor FileLogHandler(string filePath);
ctor FileLogHandler(string filePath, bool overwrite = false);

After:

ctor FileLogHandler(string filePath);
ctor FileLogHandler(string filePath, bool overwrite = false, bool someOtherArg = false);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-dotnet help wanted Issues looking for contributions I-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants