-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Add DbException.IsTransient property #38837
Add DbException.IsTransient property #38837
Conversation
…eration may succeed if tried again.
… DbException reference class
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
Tagging subscribers to this area: @roji, @ajcvickers |
src/libraries/System.Data.Common/src/System/Data/Common/DbException.cs
Outdated
Show resolved
Hide resolved
Could you please also add a test? |
Hello @jkotas, I'm aware that for a new pull request Microsoft would like to see a test for the functionality. However, this is adding a property to the DbException class with a default implementation. The idea is providers like Microsoft.Data.SqlClient, MySqlConnector, NpgSql will actually implement that property. With that in mind I didn't see the value of a test because the derived class is expected to set that property. If there is something specific you have in mind I can do the test |
The test can be very simple: Just call the base class property and check that it returns false. |
Could you please resolve the conflict? |
src/libraries/System.Data.Common/tests/System/Data/Common/DbExceptionTests.cs
Show resolved
Hide resolved
src/libraries/System.Data.Common/tests/System/Data/Common/DbExceptionTests.cs
Show resolved
Hide resolved
…ceptionTests.cs Co-authored-by: Stephen Toub <stoub@microsoft.com>
…ceptionTests.cs Co-authored-by: Stephen Toub <stoub@microsoft.com>
@rgarrison12345 thanks for this! Can you please squash your commits and rebase them so it's a single, clean commit on top of the latest master branch commits (no merge commits)? Note that there's also #35601 for adding SqlState to DbException. There was also a proposal in the design discussion to provide a default implementation of IsTransient by looking at SqlState, and identifying some specific values which are inherently transient. This doesn't have to be done right away though. |
…com/rgarrison12345/runtime into Issue-34817-DbException-IsTransient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the spurious DbException constructors introduced here. It's also best to squash these 13 commits into a single one on top of master (no merge commits necessary).
protected DbException() { } | ||
protected DbException(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) { } | ||
protected DbException(string message) { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this slipped in by mistake, please remove.
@rgarrison12345 any reason for closing this? If you intend to open a new PR for this, please reopen this one instead, and force-push to its branch (a new PR would require new approval etc.). Otherwise please let me know and I'll do this functionality. |
Hello @roji, I'll admit I'm fairly new with the Git workflow. I'm more familiar with TFS. When I did the pull request originally I thought I had latest, but it's apparent I didn't and that was causing some issues. I was closing this to open a new one to start over fresh. Can you do this functionality? I want to contribute, I think I need to learn more about git in the meantime. |
Sure, I'll soon submit a PR for both this and #35601. Git is indeed non-trivial for newcomers, but it's well-worth the investment to learn it! Hope to see more contributions from you soon. |
Replaced by #39157 |
Fixes #34817