Misc code cleanup #313
Misc code cleanup #313
Conversation
|
Yeah I'm the same way, I like to have things consistent. I've just been cleaning up files as I add enhancements or bug fixes (moving the usings above the namespacing, make sure the indentation is right, etc). We don't have any concept of standardization right now, so I wouldn't be opposed to something like an editorconfig |
|
I don't really have any issues with this PR, my only worry is our current release methodology which we've already discussed and will be sticking with. If we accept a PR that is purely refactoring with no benefit aside from readability, the consumer will get pinged to update their references but won't be pulling anything of value. Maybe that's not a huge deal, but it just seems a little wrong, at least to me. While I really appreciate code cleanup, I'm sort of leaning towards an approach where PRs should fix a bug or add a feature, and while we're in there doing it, perform a code clean up. Thoughts? #295 and #308 for some additional reading on the release strategy. |
| } | ||
|
|
||
| this.instance = instance; | ||
| this.instance = instance ?? throw new ArgumentNullException("instance"); |
There was a problem hiding this comment.
nameof(instance) - same for other changed files
|
Re "release strategy" - I think it's gonna be hard to decide on what "brings value" - so I'd suggest to just merge whatever we think is a sensible change (which IMO this PR is) |
|
Also you're going to see some merge conflicts with #315 -- were getting rid of the if block for Unix checks. |
|
@jpreese after reading over #295 and #308, here's my $0.02 since you asked for it :) I understand the potential concern of "over notifying" people about new package updates, but out of all of the bigger projects I've worked on, we've always more work than we can handle and only upgraded NuGet packages when we absolutely had to, so I wouldn't be too worried about a lot of "newer" versions of a package accumulating particularly when they're not major version updates. Have we had specific feedback before about having too many notifications? From a contributor perspective, I'm not a fan of having to including refactoring as part of other changes for two reasons:
With that said, I'm going to step down off my soap box and fix @fgreinacher's feedback :) If you'd prefer to merge this with something else then I can wrap it up with #311 although I've got some other cleanup to do on that PR before it will be ready to merge. |
TestingHelpers/MockFileSystem.cs
Outdated
|
|
||
| public IFileSystemWatcherFactory FileSystemWatcher { get; } | ||
|
|
||
| public PathVerifier PathVerifier |
There was a problem hiding this comment.
This could be
public PathVerifier PathVerifier => pathVerifier;to make it a bit more consistent with the new getters. Do we care about the newline between each property?
There was a problem hiding this comment.
Fixed! I'm not too familiar with the [NonSerialized] attribute, but could we drop the private readonly variable entirely and have:
[NonSerialized]
public PathVerifier PathVerifier { get; }
I personally like the newline between the properties, but I prefer consistency to any personal preference I have :)
There was a problem hiding this comment.
Not a huge deal just to leave it as going to be a matter of preference.. and we don't have any standards at the moment.
All said and done, I can be down for these PRs as I am a refactoring fiend. |
|
Looks good to me! 🎉 |
|
Merging - thanks for taking care! |
|
My first PR merged, woo-hoo 🍾 |
3 participants
Leveraging some new C# features to simplify the code.
Do we have style guidelines we're following? The neat freak in me was noticing inconsistencies but didn't know how to clean them up. For example, usings are mixed between being outside and inside the namespace.