- Notifications
You must be signed in to change notification settings - Fork311
Merge | Fix/commands null#3310
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
@dotnet-policy-service agree company="VTEX" |
It looks like a good idea - the change is pretty simple. However, we want to make sure that these get caught by our localization process (and that we have consistent error messaging). Could you put the error message in our /resources/string.resx file and reference that when throwing the exception, please? |
/azp run |
| Azure Pipelines successfully started running 2 pipeline(s). |
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.
Use string resources to support localization.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBatch.cs OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
/azp run |
| Azure Pipelines successfully started running 2 pipeline(s). |
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.
Found the test failure! One character change, should be easy fix. :)
| { | ||
| get | ||
| { | ||
| return ResourceManager.GetString("ADP_NoSqlBatchCommandList", resourceCulture); |
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 real goodexcept ... Either this line should beADP_NoSqlBatchCommandsList (with ans) OR the resx file should beADP_NoSqlBatchCommandList (without ans). I personally would vote for the second choice
As it stands right now, the mismatch is causing the unit test to fail due to ArgumentNullException coming from looking up the resx.
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.
Good catch! Great suggestion. Just fixed.
/azp run |
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.
If the build passes, I'm on board with this change 🚢
| Azure Pipelines successfully started running 2 pipeline(s). |
@benrr101 Do you have any clue about tests? It seems the results are not good. =\ |
Looks like some network blips. I'm re-running the failed tests. |
e5cef80 intodotnet:mainUh oh!
There was an error while loading.Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## main #3310 +/- ##==========================================- Coverage 72.94% 0 -72.95%========================================== Files 298 0 -298 Lines 57002 0 -57002 ==========================================- Hits 41579 0 -41579+ Misses 15423 0 -15423
Flags with carried forward coverage won't be shown.Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Currently the method is returningObject reference not set to an instance of an object when we try to run a batch without commands. This PR returnsSqlBatchCommand list has not been initialized instead.
I'd appreciate a CI & Test run.