XML doc comments coverage (edits & additions)

[Ste1io]

A couple notes regarding the changes on this branch:

• All multi-line comment sections (<section>...</section>, <remarks>...</remarks>, etc) have been combined into single lines.
• Indented comments had the preceeding spaces removed (/// foobar => /// foobar).
• The order of some xml doc comment blocks were rearranged based on the following ordering rules:

  <!-- methods -->
  <section/>
  <remarks/>
  <typeparam/>
  <param/>
  <returns/>
  <exception/>
  
  <!-- properties -->
  <section/>
  <remarks/>
  <value/> <!-- used instead of "returns" tag -->

The above changes were to enable me to more easily parse through the existing comments to identify and fix any spelling errors or needed modifications. Also, it should make reviewing the diffs less cumbersome. Not to mention editing or writing comments while maintaining the appropriate line breaks is a pain in the @$$. :) Easier to just reapply the formatting when everything is ready for merging.

While my intention was to just add the missing doc comments causing the compiler warnings, it made more sense for me to do a full thorough review while I was at it. Ensuring consistency across the board will make implementing a static documentation site for GitHub pages (using docfx or something similar) that's generated from the code sooo much easier, if we choose to do that (imo, it would be a good idea). Most importantly, though, it's been very beneficial for me personally catching up with the changes to the library since the single-file days, which is helping shape out the direction for some of the new documentation tremendously.

Apologies in advance for such a huge amount of code for you guys to look over. I plan on squashing the commits when it's ready for merging, but leaving them as is in the meantime so that reviewing related changes between two consecutive commits is possible. Most of the changes in the first commit will evaporate with the final commit, but actual changes made to the existing comments I felt should be easily reviewed without formatting changes or additions crowding up the diffs.

[pleb]

@Ste1io thanks for fixing this up :)

[Ste1io]

Morning gents, wrapping up the remaining xml doc comments for all things async before the weekend. Do you want the leading spaces for summary and remarks sections added back in, or left as is? Also, what's the magic number for the column breaks?

[pleb]

@Ste1io I don't mind regarding leading space or not, so long as it's consistent. Regarding doc comments length, pick something that works for you but isn't too constrained for everyone... maybe something between 80 - 160 etc

[Ste1io]

@pleb will do. Plan on wrapping this PR up over the weekend or early next week. What do you want me to do about the changes to the doc tests?

[Ste1io]

Aside from some minor fix-ups and any additional changes following code review, xml doc comments are finished. So as not to contribute to the loss of your sanity, I batched all relocated code into two commits, so the actual changes can be easily diffed without making your eyes bleed. Expect these commits to get pushed to remote shortly.

[Ste1io]

e294f5b is the best commit to compare diffs for additional changes after this point, as everything is a line-for-line match.

https://github.com/CollaboratingPlatypus/PetaPoco/pull/682/files/e294f5bb06812d28ea90c6f96e59f44cec010fc3..5cd6bdbf208ede225dce461d44666bf6d9240092

[Ste1io]

I've got a dozen or so issues that surfaced while doing the doc comments. They all need to be addressed, but they aren't related to my changes. I started adding them in this thread, but as some of them are breaking changes (changes with interface contracts, as one example), I'd rather avoid holding up this PR until a major release because preexisting issues are associated with it.

Any preference with how I go about them? Bulk them up in a couple of issues, or an issue for each? They're all fairly easy to fix, and I'll be happy to front the grunt work with fixing them (I have a local branch with many of the fixes already implemented, as it stands), but pushing to a release build would require a major version bump.

For context, here's a portion of the todo comment dump so you know what I'm referring to (objective observations only; opinionated ideas are omitted for another discussion on another day :)):

21 results - 7 files

PetaPoco\Database.cs:
   579:         // TODO: [BREAKING] Not in IDatabase interface: `OnException(Exception)`, to allow GridReader's DI impl to accept IDatabase (IAsyncReader is able to correctly use the interface for it's IOC because it doesn't require the OnException))
   895:         // TODO: [BUGFIX] `QueryAsync(CancellationToken, string, object[])` takes a caller-provided CancellationToken, but uses a default empty token for wrapped call
   949:         // TODO: [BUGFIX] `QueryAsync(Action<T>, CancellationToken, string, object[])` takes a caller-provided CancellationToken, but uses a default empty token for wrapped call
  1340:         // TODO: [BUGFIX] `FetchAsync(CancellationToken, string, object[])` takes a caller-provided CancellationToken, but uses a default empty token for wrapped call
  1622:         // TODO: [BUGFIX] `SkipTakeAsync(long, long, Sql)` should forward call to an overload that receives a CancellationToken
  3220:         // TODO: [BUGFIX] `CommandHelperAsync(CancellationToken, DbCommand, Func<CancellationToken, DbCommand, Task<object>>` takes DBCommand type; inconsistent with synchronous version `CommandHelper(IDbCommand, Func<IDbCommand, object>)` which correctly uses IDbCommand type

PetaPoco\IAsyncReader.cs:
  23:         // TODO: [BREAKING] Missing overload: `Task<bool> ReadAsync(CancellationToken)`

PetaPoco\IConnection.cs:
  42:         // TODO: [BREAKING] Missing overload: `Task OpenSharedConnectionAsync(CancellationToken)`

PetaPoco\IDatabase.cs:
  105:         // TODO: [BREAKING] Missing: `AbortTransactionAsync()`
  115:         // TODO: [BREAKING] Missing: `CompleteTransactionAsync()`
  141:         // TODO: [BREAKING] Missing: `event EventHandler<DbConnectionEventArgs> ConnectionOpening`

PetaPoco\IQuery.cs:
  472:         // TODO: [BREAKING] Missing overload: `bool IQuery.Exists<T>(Sql);`

PetaPoco\IQueryAsync.cs:
  478:         // TODO: [BREAKING] Missing overload: `IQueryAsync.ExistsAsync<T>(Sql);`
  495:         // TODO: [BREAKING] Missing overload: `IQueryAsync.ExistsAsync<T>(CancellationToken, Sql);`

PetaPoco\Utilities\AsyncReader.cs:
  62:         // TODO: [BREAKING] Not implemented: `ReadAsync(CancellationToken)`

For future reference, line references above correspond with 5cd6bdb.

There's also a heap of "not broken but worth discussing" notes that would all fit into at least a partial refactor (v7?!?), which I've omitted here, as they should probably best be proposed with a discussion so everything is somewhat coherent.

[edit] Not sure what I was thinking earlier, but passing in a cancellation token for abort/close/cleanup isn't going to be very productive. 🤦🏻‍♂️ Removed those from the dump above.

[Ste1io]

After @asherber is finished looking over anything and any changes following are sorted, I'll update this branch with the text wrap and we should be all set. You want me to squash down these commits on that final commit, or leave them as is and let you handle that how you want when you merge in the PR?

[Ste1io]

@pleb and @asherber I made one final commit that fixes some terminology and unifies the doc comments for matching params across the code base. The only change left is to rewrap the doc comments blocks; I'm holding off on that until I get a confirmation that this PR is ready to merge in since comparing diffs trends to be easier without.

[pleb]

All good, @Ste1io. If the tests are passing, I say it's good to go

[Ste1io]

All good, @Ste1io. If the tests are passing, I say it's good to go

All Sqlite tests pass. I need to get my docker setup sorted before I can test the others, but will advise as soon as that's done.

[Ste1io]

Finally got around to restarting my pc (a rare occurrence) and reconfiguring virtualization in BIOS again since replacing my mobo and processor earlier this year, so I'm good to go with docker once more.

Which brings me to my question... How long has it been since the full test suite has been run for all dbms?

[Ste1io]

All tests pass. Ready to merge @pleb

[Ste1io]