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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CA1816: correct idisposable implementation #2780

Merged
merged 1 commit into from
Jul 29, 2021
Merged

Conversation

Evangelink
Copy link
Contributor

Fix implementation of the disposable pattern.

Description

See Implementing Dispose and CA1816

@@ -33,8 +33,19 @@ protected RepositoryFixtureBase(IRepository repository)
/// <summary>
/// Performs application-defined tasks associated with freeing, releasing, or resetting unmanaged resources.
/// </summary>
public virtual void Dispose()
public void Dispose()
Copy link
Contributor Author

@Evangelink Evangelink Jul 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Proper" implementation of the disposable pattern.

Note that this change could be considered as breaking change if you consider this class as part of your public API.

@@ -13,7 +13,7 @@ public interface IAssemblyInfoFileUpdater : IVersionConverter<AssemblyInfoContex
{
}

public class AssemblyInfoFileUpdater : IAssemblyInfoFileUpdater
public sealed class AssemblyInfoFileUpdater : IAssemblyInfoFileUpdater
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When class wasn't inherited I went with the option of sealing the class but we could also go for the disposable pattern implementation.

Note that this change could be considered as breaking change if you consider this was part of your API.

Copy link
Member

@asbjornu asbjornu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine. I can't imagine anyone using GitTools.Testing beside us, so I don't think there are any consumers to care for. If there are, perhaps with this change we'll find out.

@arturcic arturcic merged commit 45f074b into GitTools:main Jul 29, 2021
@mergify
Copy link
Contributor

mergify bot commented Jul 29, 2021

Thank you @Evangelink for your contribution!

@arturcic arturcic added this to the 5.x milestone Jul 29, 2021
@Evangelink Evangelink deleted the CA1816 branch July 29, 2021 15:03
@arturcic arturcic modified the milestones: 5.x, 5.6.12 Aug 14, 2021
@arturcic
Copy link
Member

🎉 This issue has been resolved in version 5.7.0 🎉
The release is available on:

Your GitReleaseManager bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants