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

RelationalDiagnostics: Naming inconsistencies #8001

Closed
NickCraver opened this issue Mar 27, 2017 · 2 comments
Closed

RelationalDiagnostics: Naming inconsistencies #8001

NickCraver opened this issue Mar 27, 2017 · 2 comments
Assignees
Labels
breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@NickCraver
Copy link
Member

I'm working on a DiagnosticListener approach in #7939 and noticed some inconsistencies in RelationalDiagnostics.cs:

  1. ConnectionError is incorrect, nameof(ConnectionClosed) (repeated)should benameof(ConnectionError)`. I've submitted PR Fix diagnostic name for RelationalDiagnostics.ConnectionError #8000 to correct this.
public const string ConnectionClosed = NamePrefix + nameof(ConnectionClosed);
public const string ConnectionError = NamePrefix + nameof(ConnectionClosed);
  1. "TransactionRolledBack" isn't title cased:
public const string TransactionRolledback = NamePrefix + nameof(TransactionRolledback);

^ this should have a capital B to be consistent with all other events. in the class, e.g.

public const string TransactionRolledBack = NamePrefix + nameof(TransactionRolledBack);

1 is an obvious bug (unless I'm nuts) and causing active harm re-firing the closed event on errors, not just closes. While there's a workaround of using the exception arg on the close listener and seeing if it's null, this is both a) not ensure to be reliable, and b) just plain wrong.

2 is minor, but a breaking change to listeners all the same. At the same time, I can't find anyone even using this yet (or someone would have noticed these things?) - should we fix it properly now?

See #7939 (comment) for additional concerns I have about the stability of these APIs. Since everyone is (as far as I know) copying these strings because they are internally restricted, there's no scenario fixed by a simple recompile. Issues like this would be better served in the event names were exposed and compilationally reliable.

@ajcvickers
Copy link
Member

@NickCraver Thanks for the PR for 1. We would also take a PR for 2, since we can take reasonable breaking changes in the 2.0 release. Would you be willing to send a PR?

@anpete
Copy link
Contributor

anpete commented Jun 26, 2017

I think this has all been taken care of with @ajcvickers rework of this area.

@anpete anpete closed this as completed Jun 26, 2017
@anpete anpete added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jun 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Projects
None yet
Development

No branches or pull requests

3 participants