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

EventLogReader.ReadEvent - documenting null return #8207

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

daiplusplus
Copy link
Contributor

Summary

The documentation for EventLogReader.ReadEvent does not indicate how to detect being at the last result (it returns null, but this isn't documented at all) - so this PR adds that detail.

Fixes #8206

@daiplusplus daiplusplus requested a review from a team as a code owner July 11, 2022 11:25
@dotnet-issue-labeler
Copy link

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.

@opbld33
Copy link

opbld33 commented Jul 11, 2022

Docs Build status updates of commit 6b3bb53:

✅ Validation status: passed

File Status Preview URL Details
xml/System.Diagnostics.Eventing.Reader/EventLogReader.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@opbld31
Copy link

opbld31 commented Jul 11, 2022

Docs Build status updates of commit 1f55fa5:

✅ Validation status: passed

File Status Preview URL Details
xml/System.Diagnostics.Eventing.Reader/EventLogReader.xml 💡Suggestion View Details

xml/System.Diagnostics.Eventing.Reader/EventLogReader.xml

  • Line 0, Column 0: [Suggestion: disallowed-html-tag - See documentation] HTML tag 'format' isn't allowed. Replace it with approved Markdown or escape the brackets if the content is a placeholder.
  • Line 0, Column 0: [Suggestion: disallowed-html-tag - See documentation] HTML tag 'format' isn't allowed. Replace it with approved Markdown or escape the brackets if the content is a placeholder.

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@ghost
Copy link

ghost commented Jul 11, 2022

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

Summary

The documentation for EventLogReader.ReadEvent does not indicate how to detect being at the last result (it returns null, but this isn't documented at all) - so this PR adds that detail.

Fixes #8206

Author: Jehoel
Assignees: -
Labels:

area-System.Diagnostics

Milestone: -

@@ -433,7 +433,7 @@
<AssemblyVersion>4.0.0.0</AssemblyVersion>
</AssemblyInfo>
<Docs>
<summary>Reads the next event that is returned from the event query in this object.</summary>
<summary>Reads and returns the next event from the event query results - or null if the end of the results has been reached.</summary>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<summary>Reads and returns the next event from the event query results - or null if the end of the results has been reached.</summary>
<summary>Reads and returns the next event from the event query results, or null if the end of the results has been reached.</summary>

@@ -464,16 +464,52 @@
</ReturnValue>
<Parameters />
<Docs>
<summary>Reads the next event that is returned from the event query in this object.</summary>
<returns>Returns an <see cref="T:System.Diagnostics.Eventing.Reader.EventRecord" /> object.</returns>
<summary>Reads and returns the next event from the event query results - or null if the end of the results has been reached.</summary>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<summary>Reads and returns the next event from the event query results - or null if the end of the results has been reached.</summary>
<summary>Reads and returns the next event from the event query results.</summary>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Methinks I should add the <returns> element back…

…but why remove the important details about null here?

<summary>Reads the next event that is returned from the event query in this object.</summary>
<returns>Returns an <see cref="T:System.Diagnostics.Eventing.Reader.EventRecord" /> object.</returns>
<summary>Reads and returns the next event from the event query results - or null if the end of the results has been reached.</summary>
<returns>Returns an <see cref="T:System.Diagnostics.Eventing.Reader.EventRecord" /> object - or a null reference.</returns>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<returns>Returns an <see cref="T:System.Diagnostics.Eventing.Reader.EventRecord" /> object - or a null reference.</returns>
<returns>Returns an <see cref="T:System.Diagnostics.Eventing.Reader.EventRecord" /> object, or a null reference if the end of the results has been reached.</returns>

This overload is equivalent to calling <see cref="M:System.Diagnostics.Eventing.Reader.EventLogReader.ReadEvent(System.TimeSpan)" /> with an <see cref="M:System.Threading.Timeout.InfiniteTimeSpan" /> timeout parameter argument.
</para>
<para>
Each time this method is called the reader will advance to the next event record in its query results or the log file being read.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Each time this method is called the reader will advance to the next event record in its query results or the log file being read.
Each time this method is called, the reader advances to the next event record in the query results or the log file.

</para>
<para>
Each time this method is called the reader will advance to the next event record in its query results or the log file being read.
When the reader reaches the end of the query results or log file the method will return null on its next call, but subsequent calls will throw an InvalidOperationException.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
When the reader reaches the end of the query results or log file the method will return null on its next call, but subsequent calls will throw an InvalidOperationException.
When the reader reaches the end of the query results or log file, the method returns <c>null</c> on its next call, and subsequent calls will throw an <see cref="T:System.InvalidOperationException" />.

When the reader reaches the end of the query results or log file the method will return null on its next call, but subsequent calls will throw an InvalidOperationException.
</para>
<para>
The returned EventRecord objects implement IDisposable and should be disposed of appropriately. To avoid leaking resources consider copying desired data from the EventRecord object into a custom class or tuple and then immediately disposing of the returned EventRecord before advancing to the next logged event.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The returned EventRecord objects implement IDisposable and should be disposed of appropriately. To avoid leaking resources consider copying desired data from the EventRecord object into a custom class or tuple and then immediately disposing of the returned EventRecord before advancing to the next logged event.
The returned EventRecord objects implement IDisposable and should be disposed of appropriately. To avoid leaking resources, consider copying desired data from the EventRecord object into a custom class or tuple and then immediately disposing of the returned EventRecord before advancing to the next logged event. The following code shows an example.

<para>
The returned EventRecord objects implement IDisposable and should be disposed of appropriately. To avoid leaking resources consider copying desired data from the EventRecord object into a custom class or tuple and then immediately disposing of the returned EventRecord before advancing to the next logged event.
</para>
<format type="text/markdown"><![CDATA[
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example should be code-fenced.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pardon my ignorance, but what is code-fencing? Also, what about the build validation warnings about <format> being invalid HTML? (…and I’m not even writing HTML, weird)

<!--<exception cref="T:System.Diagnostics.Eventing.Reader.EventLogException"></exception>-->
<exception cref="T:System.Diagnostics.Eventing.Reader.EventLogReadingException"></exception>
<exception cref="T:System.Diagnostics.Eventing.Reader.EventLogInvalidDataException"></exception>
<exception cref="T:System.InvalidOperationException">Thrown when ReadEvent is called again after a previous call returned null.</exception>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<exception cref="T:System.InvalidOperationException">Thrown when ReadEvent is called again after a previous call returned null.</exception>
<exception cref="T:System.InvalidOperationException">ReadEvent was called again after a previous call returned null.</exception>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m unsure if I should retain the <exception> reference to EventLogReaderException even though it’s redundant as the other exception types are subclasses thereof) - is it possible to render a hierarchical exception type list here?

@@ -515,7 +551,48 @@
<param name="timeout">The maximum time to allow the read operation to run before canceling the operation.</param>
<summary>Reads the next event that is returned from the event query in this object.</summary>
<returns>Returns an <see cref="T:System.Diagnostics.Eventing.Reader.EventRecord" /> object.</returns>
<remarks>To be added.</remarks>
<remarks>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make all the same changes below.

@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EventLogReader.ReadEvent does not document null-returning behaviour, IDisposable info, nor give usage example
6 participants