FileSink constructor should guard against empty/whitespace paths

[reubenalfred]

Hi,

I was just browsing the FileSink file and noticed that the path parameter is only tested for null and doesn't include a test for NullOrWhiteSpace.

Further down in the constructor, it is tested to see if the path's dir is null or white space and creates the directory if the path string is valid, however, no action is specified when the string is empty.

it is an internal constructor, perhaps it may not be that important. i thought i'd let you know...

regards, reuben

[reubenalfred]

if it needs fixing ill be happy to do that :)

[nblumhardt]

Hi @reubenalfred! string.IsNullOrWhiteSpace("") is true so I don't think the empty string case is neglected here.

[Micke3rd]

action is specified when the string is empty

string.IsNullOrWhiteSpace:
"true if the value parameter is null or Empty, or if value consists exclusively of white-space characters."
doc

[reubenalfred]

If it is null or white space it won't create the directory sure but it will still attempt to execute the rest of the code without a definite directory.

path too should be tested for null or white space at the top instead of just testing for null alone.

[reubenalfred]

action is specified when the string is empty

string.IsNullOrWhiteSpace: "true if the value parameter is null or Empty, or if value consists exclusively of white-space characters." doc

Where is the action specified? Please show me??

[Micke3rd]

If it is null or white space it won't create the directory sure but it will still attempt to execute the rest of the code without a definite directory.

ah got it. Yes, I thought you mean the create directory part.

path too should be tested for null or white space at the top instead of just testing for null alone.

Yes, would be an improvement.
System.IO.File.Open itself contains a ThrowIfNullOrEmpty(path), which you would probably copy to the top line.
It would therefore be redundant, but also not because the check at CreateDirectory can then be omitted.

if it needs fixing ill be happy to do that :)

do it ... I would be surprised if the team here turns down the help.

[reubenalfred]

Cool will do that, I personally like to test for obvious and explicitly throw exceptions rather than rely on the API's otherwise it is too much to remember what throws what!

[bartelink]

@reubenalfred I'm spring cleaning (e.g. #297 (comment) #263 (comment))

Before any work takes place to add checks, it'd be good for us to collectively get #258 over the line, as it may already be address some of these issues.

In short, as:

• we want/need all null checks to be complete and correct
• if there is an issue that clearly and concisely documents a desired behavior change wrt empty strings in some cases, then a PR can make the requisite changes
• in general whitespace trimming is not something that we want to do

The next steps here are to make sure your concerns are covered by either
a) commenting on #258
b) writing a new issue detailing the needs (no large screeshots please though ;))