refactor: internal refactors#11
Conversation
|
Hi @iterma, thanks a lot for your contributions to this repo. I see that you opened multiple pull requests but this one(#11) is actually a superset of #10 and #9. I am going to close #9 and #10 in favor of this pull request, which simplifies the review process. Generally speaking, one pull request should only do one thing. |
| public object ConvertBack(object value, Type targetType, object parameter, CultureInfo culture) | ||
| { | ||
| return (bool)value ? Visibility.Visible : Visibility.Collapsed; // Not used. | ||
| } |
There was a problem hiding this comment.
This implementation of ConvertBack is not correct. What ConvertBack should do is converting from Visibility to bool, which is useless for our case. (FYI: It is useful in two-way bindings)
| public object ConvertBack(object value, Type targetType, object parameter, CultureInfo culture) | ||
| { | ||
| return (bool)value ? Visibility.Collapsed : Visibility.Visible; // Not used. | ||
| } |
There was a problem hiding this comment.
The same issue here. Wrong implementation here. And definitely no need to implement it.
| public class CommandFailedException : Exception | ||
| namespace WSLDiskShrinker.Common; | ||
|
|
||
| [Serializable] |
There was a problem hiding this comment.
I don't see the point of making it Serializable. Can you explain it?
| this.FailedProcess = p; | ||
| } | ||
|
|
||
| // Implement exception constructors (not used). |
There was a problem hiding this comment.
Why adding implementations that are not used 😂?
|
Sorry about the late response. I saw your message but I'm out sailing for a few days, one of the reasons I prepared several PRs but shouldn't have sent them off. I have my computer with me but the weather is too nice 😎☀️ Will return after the weekend. // Thanks, anyway, for showing interest in the PRs. |
Thanks for your continued effort in this repo. |
2 participants
Added constructor for simplicity and less redundancy (also less mutable 😉).