Skip to content

Give DirectoryHelper.DeleteDirectory() a more meaningful name #980

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

Closed
nulltoken opened this issue Feb 24, 2015 · 3 comments
Closed

Give DirectoryHelper.DeleteDirectory() a more meaningful name #980

nulltoken opened this issue Feb 24, 2015 · 3 comments

Comments

@nulltoken
Copy link
Member

From #977 (comment)

Second, would it make sense to rename the top-level DeleteDirectory() method into TryDeleteDirectory() as well? My rationale is that it (1) does not ensure directories are indeed deleted and (2) does not provide any direct feedback to the caller (only BaseFixture.Dispose() at the moment) that it could not delete a directory.

Why not? However, by convention, TryXxxx methods expose a certain signature (a returned bool and an in out param). I'd prefer using a name that doesn't conflict with a known pattern.

@nulltoken
Copy link
Member Author

/cc @ThomasBarnekow

@ThomasBarnekow
Copy link
Contributor

@nulltoken Having looked at the TryParse pattern, we could potentially do the following:

// This is a new method, wrapping the existing ones and making sure it does not throw.
// BaseFixture.Dispose() would call this one if we wanted to make sure tests don't fail.
// If you want failed tests as you said, Dispose would not call this one but the next one.
public static bool TryDeleteDirectory(string path);

// This is our current DeleteDirectory() method. It can theoretically throw. 
// Thus, we should keep the name.
public static void DeleteDirectory(string path);

// This is our current TryDeleteDirectory() method which throws under rare circumstances. 
// This is why I'd propose to change the name. If, and only if, you'd call TryDeleteDirectory() 
// from the Dispose() method, we could talk about moving that final trace message to 
// TryDeleteDirectory(). Otherwise, there would be no change.
private static void DeleteDirectory(string directoryPath, int maxAttempts, int initialTimeout, int timeoutFactor)

ThomasBarnekow added a commit to ThomasBarnekow/libgit2sharp that referenced this issue Apr 14, 2015
Rename TryDeleteDirectory(string, int, int, int) to DeleteDirectory(...)
because this method can still throw exceptions and is not fully in line
with the try-parse pattern.

Introduce TryDeleteDirectory(string) : bool as a try-parse pattern-compliant
option to delete directories recursively without throwing exceptions.

Remove DeleteSubdirectories(string) method because it is not used anywhere.

Use 'var' keyword consistently across all methods.

Related to libgit2#980
ThomasBarnekow added a commit to ThomasBarnekow/libgit2sharp that referenced this issue Apr 15, 2015
Rename TryDeleteDirectory(string, int, int, int) to DeleteDirectory(...)
because this method can still throw exceptions and is not fully in line
with the try-parse pattern.

Introduce TryDeleteDirectory(string) : bool as a try-parse pattern-compliant
option to delete directories recursively without throwing exceptions.

Remove DeleteSubdirectories(string) method because it is not used anywhere.

Use 'var' keyword consistently across all methods.

Related to libgit2#980
ThomasBarnekow added a commit to ThomasBarnekow/libgit2sharp that referenced this issue Apr 16, 2015
Rename TryDeleteDirectory(string, int, int, int) to DeleteDirectory(...)
because this method can still throw exceptions and is not fully in line
with the try-parse pattern.

Introduce TryDeleteDirectory(string) : bool as a try-parse pattern-compliant
option to delete directories recursively without throwing exceptions.

Remove DeleteSubdirectories(string) method because it is not used anywhere.

Use 'var' keyword consistently across all methods.

Related to libgit2#980
ThomasBarnekow added a commit to ThomasBarnekow/libgit2sharp that referenced this issue Apr 17, 2015
Rename TryDeleteDirectory(string, int, int, int) to DeleteDirectory(...)
because this method can still throw exceptions and is not fully in line
with the try-parse pattern.

Introduce TryDeleteDirectory(string) : bool as a try-parse pattern-compliant
option to delete directories recursively without throwing exceptions.

Remove DeleteSubdirectories(string) method because it is not used anywhere.

Use 'var' keyword consistently across all methods.

Related to libgit2#980
ThomasBarnekow added a commit to ThomasBarnekow/libgit2sharp that referenced this issue Apr 18, 2015
Rename TryDeleteDirectory(string, int, int, int) to DeleteDirectory(...)
because this method can still throw exceptions and is not fully in line
with the try-parse pattern.

Introduce TryDeleteDirectory(string) : bool as a try-parse pattern-compliant
option to delete directories recursively without throwing exceptions.

Remove DeleteSubdirectories(string) method because it is not used anywhere.

Use 'var' keyword consistently across all methods.

Related to libgit2#980
@nulltoken
Copy link
Member Author

Fixed by #1022

@nulltoken nulltoken added this to the v0.22 milestone Apr 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants