Fspes 53 #72
Fspes 53 #72
Conversation
WalkthroughStandardized async awaits to use ConfigureAwait(false) across multiple Frends.MicrosoftSQL packages, bumped package versions and consolidated csproj TargetFrameworks to a single net6.0 TargetFramework; changelogs and minor formatting/doc edits included. No public API signature changes. Changes
Sequence Diagram(s)(omitted — changes standardize await usage and do not introduce new multi-component control flows) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
Frends.MicrosoftSQL.BatchOperation/Frends.MicrosoftSQL.BatchOperation/BatchOperation.cs (2)
21-21: Make the task classstatic.The class
MicrosoftSQLcontains only static members (static constructor, static methods) and should be declared aspublic static classto comply with Microsoft C# coding standards and the FT0004 rule for Frends task classes.
40-40: Add Error object to Result class with Message and AdditionalInfo properties.The Result class is missing the Error object structure required by coding guidelines. Currently it only has an
ErrorMessagestring property, but task result classes must include an Error object withMessageandAdditionalInfoproperties. This is causing the FT0011 pipeline failure.Frends.MicrosoftSQL.BulkInsert/Frends.MicrosoftSQL.BulkInsert/BulkInsert.cs (3)
17-17: Pipeline failure: Task class must be static.The pipeline error
FT0004: Task class 'MicrosoftSQL' should be staticindicates this class needs thestaticmodifier.Proposed fix
-public class MicrosoftSQL +public static class MicrosoftSQL
26-27: Pipeline failure: Result class needs structured Error property.The pipeline error
FT0011: Class should include a 'Error' propertyindicates theResultclass inDefinitions/Result.csneeds modification. Per coding guidelines, task result classes should include an "Error object with Message and AdditionalInfo" rather than a simpleErrorMessagestring.The
Resultclass currently hasstring ErrorMessagebut should have anErrorobject:public class Error { public string Message { get; private set; } public string AdditionalInfo { get; private set; } internal Error(string message, string additionalInfo) { Message = message; AdditionalInfo = additionalInfo; } }Then update
Resultto useError Errorinstead ofstring ErrorMessage.
3-3: Remove unused import.
Microsoft.IdentityModel.Abstractionsis not used anywhere in this file and should be removed to comply with C# code cleanliness standards.Frends.MicrosoftSQL.ExecuteQuery/Frends.MicrosoftSQL.ExecuteQuery/ExecuteQuery.cs (1)
17-17: Make the task class static to satisfy FT0004.Line 17 defines a non-static task class, which fails the build rule. Please mark it static.
✅ Proposed fix
-public class MicrosoftSQL +public static class MicrosoftSQLFrends.MicrosoftSQL.ExecuteProcedure/Frends.MicrosoftSQL.ExecuteProcedure/ExecuteProcedure.cs (2)
18-18: Make the task class static to satisfy FT0004.Line 18 defines a non-static task class, which fails the build rule. Please mark it static.
✅ Proposed fix
-public class MicrosoftSQL +public static class MicrosoftSQL
93-95: Update Result to include an Error object (Message + AdditionalInfo).Lines 93–95 and 103–109 construct
Result, but the build rule FT0011 (and coding guidelines) require a dedicatedErrorobject withMessageandAdditionalInfoinstead ofErrorMessagestring. Please updateDefinitions/Result.csaccordingly and adjust these constructions.
As per coding guidelines, Result types must include a flatErrorobject withMessageandAdditionalInfo.Also applies to: 103-109
🤖 Fix all issues with AI agents
In
`@Frends.MicrosoftSQL.ExecuteQuery/Frends.MicrosoftSQL.ExecuteQuery/ExecuteQuery.cs`:
- Around line 105-107: The Result constructor calls must be changed to use the
new flat Error object (with Message and AdditionalInfo) instead of the old
ErrorMessage string: update Definitions/Result.cs to add/require an Error
property/class (e.g., Error { string Message; string AdditionalInfo; }) and then
replace the two places that construct Result (the occurrences around result =
new Result(true, dataReader.RecordsAffected, null, await LoadData(...)) and the
other construction near the later branch) to pass an Error instance (null for no
error) or new Error { Message = "...", AdditionalInfo = "..." } where
appropriate; ensure the Result constructor signature and all call sites
(including any code that used ErrorMessage) are updated to match the new Error
type and populate AdditionalInfo with contextual details.
In
`@Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile/Definitions/CsvFileWriter.cs`:
- Line 164: Remove the stray space before the member-access operator on the
CsvFileWriter usage: locate the call to csvWriter .WriteField(formattedValue,
false) (in CsvFileWriter.cs) and change it to use
csvWriter.WriteField(formattedValue, false) so the dot directly follows the
variable name and the StyleCop SA1019 warning is resolved.
🧹 Nitpick comments (1)
Frends.MicrosoftSQL.ExecuteProcedure/CHANGELOG.md (1)
9-13: Pre-existing format issue: missing section header in 2.2.0 entry.The 2.2.0 entry is missing the
### Changed(or### Added/### Fixed) header before the list items. Per Keep a Changelog format, each version should have categorized sections. Consider fixing this while you're updating the file.Suggested fix
## [2.2.0] - 2024-12-16 +### Changed + - Added method to form JToken from the SqlDataReader so that SqlGeography and SqlGeometry typed objects can be handled.
Frends.MicrosoftSQL.ExecuteQuery/Frends.MicrosoftSQL.ExecuteQuery/ExecuteQuery.cs
Show resolved
Hide resolved
...ftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile/Definitions/CsvFileWriter.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Frends.MicrosoftSQL.ExecuteQueryToFile/Frends.MicrosoftSQL.ExecuteQueryToFile/Definitions/CsvFileWriter.cs (2)
33-50: Add missing XML documentation to fix pipeline failure.The pipeline is failing with error FT0014 because the public method
SaveQueryToCSVis missing required XML documentation.Proposed fix
+ /// <summary> + /// Executes the SQL query and saves the results to a CSV file. + /// </summary> + /// <param name="cancellationToken">Token to monitor for cancellation requests.</param> + /// <returns>A Result containing the row count and output file information.</returns> public async Task<Result> SaveQueryToCSV(CancellationToken cancellationToken)The addition of
ConfigureAwait(false)on line 43 is a good improvement for library code to avoid capturing the synchronization context.
84-97: Remove dead code: unused method call.Line 87 calls
options.GetFieldDelimiterAsString()but discards the result. This appears to be dead code that should be removed.Proposed fix
if (dotnetType == typeof(string)) { var str = (string)value; - options.GetFieldDelimiterAsString(); str = str.Replace("\"", "\\\""); str = str.Replace("\r\n", " "); str = str.Replace("\r", " "); str = str.Replace("\n", " ");
Review Checklist
Summary by CodeRabbit
Chores
Refactor
Documentation
Style
✏️ Tip: You can customize this high-level summary in your review settings.