Skip to content

Conversation

chrisdunelm
Copy link
Contributor

No description provided.

/// </remarks>
public abstract partial class BigQueryClient
public abstract partial class BigQueryClient : IDisposable

This comment was marked as spam.

@jskeet
Copy link
Collaborator

jskeet commented Nov 6, 2017

@benwulfe: No, because it's not actually a requirement. As documented, it's something you should do in some situations, but it's not only allowing you to be more efficient. Any code that doesn't dispose of the client will work just as well as it did before.

Copy link
Collaborator

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

There's an extra client to do: AdvancedTranslationClient.

@chrisdunelm
Copy link
Contributor Author

AdvancedTranslationClient done

@benwulfe
Copy link
Contributor

benwulfe commented Nov 6, 2017

If you have singletons that implement IDisposable, not disposing it is a pretty common (if maybe not entirely correct) usage pattern. I'd argue this caveat doesn't really make it special or unique.

@benwulfe
Copy link
Contributor

benwulfe commented Nov 6, 2017

I guess all I was saying is that the very existence of having an object that implements IDisposable that is not disposed is likely to create a lint error where it was ok before.
linters will not be able to read your comments.

@jskeet
Copy link
Collaborator

jskeet commented Nov 6, 2017

Well, I'd argue that the expected behaviour at that point is for the developer to read the linter error, read the doc comment then, and configure the linter appropriately. If the linter can't be configured, I think that's a bigger problem. (And how does it cope with HttpClient and Task for example?)

I really believe that bumping the major version number would be a mistake here, giving an unrealistic impression of breaking.

@chrisdunelm chrisdunelm merged commit 9f2bde3 into googleapis:master Nov 6, 2017
@chrisdunelm chrisdunelm deleted the restdispose branch November 6, 2017 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants