Re: [PR] TINKERPOP-3029 Fix enumeration for .NET 8 [tinkerpop]
FlorianHockmann merged PR #2424: URL: https://github.com/apache/tinkerpop/pull/2424 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] TINKERPOP-3029 Fix enumeration for .NET 8 [tinkerpop]
xiazcy commented on PR #2424: URL: https://github.com/apache/tinkerpop/pull/2424#issuecomment-1888079847 VOTE +1 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] TINKERPOP-3029 Fix enumeration for .NET 8 [tinkerpop]
vkagamlyk commented on PR #2424: URL: https://github.com/apache/tinkerpop/pull/2424#issuecomment-1883908255 thank you @FlorianHockmann! VOTE+1 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] TINKERPOP-3029 Fix enumeration for .NET 8 [tinkerpop]
FlorianHockmann commented on PR #2424: URL: https://github.com/apache/tinkerpop/pull/2424#issuecomment-1878426662 I added a CHANGELOG / upgrade docs entry and applied the suggestion by @vkagamlyk. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] TINKERPOP-3029 Fix enumeration for .NET 8 [tinkerpop]
FlorianHockmann commented on code in PR #2424:
URL: https://github.com/apache/tinkerpop/pull/2424#discussion_r1441466689
##
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/DefaultTraversal.cs:
##
@@ -82,21 +82,22 @@ public bool MoveNext()
}
private bool MoveNextInternal()
-{
+{
if (_fetchedNext) return _nextAvailable;
+
+if (!_nextAvailable || _nextAvailable &&
TraverserEnumerator.Current?.Bulk == 0)
Review Comment:
@EricSites sure, but `DefaultTraversal` is an `IEnumerator` itself so
`GetCurrent()` just forwards to `TraverserEnumerator.Current`. If it's illegal
to call `TraverserEnumerator.Current` before calling
`TraverserEnumerator.MoveNext()`, then it's also illegal to call `GetCurrent()`
before calling `MoveNext()`.
We could of course use a flag to check whether enumeration has been started
in `GetCurrent()` and return `null` otherwise, but I don't think it's a good
idea to deviate from the default behavior of .NET enumerators.
Did you see `GetCurrent()` in any stack trace?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] TINKERPOP-3029 Fix enumeration for .NET 8 [tinkerpop]
EricSites commented on code in PR #2424:
URL: https://github.com/apache/tinkerpop/pull/2424#discussion_r1441072113
##
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/DefaultTraversal.cs:
##
@@ -82,21 +82,22 @@ public bool MoveNext()
}
private bool MoveNextInternal()
-{
+{
if (_fetchedNext) return _nextAvailable;
+
+if (!_nextAvailable || _nextAvailable &&
TraverserEnumerator.Current?.Bulk == 0)
Review Comment:
This code will also throw an exception if called without first starting the
iterator: You will need to track that the iterator has not been started.
```.csharp
private object GetCurrent()
{
// Use dynamic to object to prevent runtime dynamic conversion evaluation
return TraverserEnumerator.Current?.Object;
}
```
Please review the new [.Net code
here](https://source.dot.net/#PresentationFramework/MS/Internal/WindowsRuntime/Generated/WinRT/Projections/IEnumerable.cs,239)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] TINKERPOP-3029 Fix enumeration for .NET 8 [tinkerpop]
vkagamlyk commented on code in PR #2424:
URL: https://github.com/apache/tinkerpop/pull/2424#discussion_r1440829828
##
gremlin-dotnet/src/Gremlin.Net/Process/Traversal/DefaultTraversal.cs:
##
@@ -82,21 +82,22 @@ public bool MoveNext()
}
private bool MoveNextInternal()
-{
+{
if (_fetchedNext) return _nextAvailable;
+
+if (!_nextAvailable || _nextAvailable &&
TraverserEnumerator.Current?.Bulk == 0)
Review Comment:
can be simplified
```suggestion
if (!_nextAvailable || TraverserEnumerator.Current?.Bulk == 0)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] TINKERPOP-3029 Fix enumeration for .NET 8 [tinkerpop]
codecov-commenter commented on PR #2424: URL: https://github.com/apache/tinkerpop/pull/2424#issuecomment-1875641877 ## [Codecov](https://app.codecov.io/gh/apache/tinkerpop/pull/2424?src=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) Report All modified and coverable lines are covered by tests :white_check_mark: > Comparison is base [(`e86eed2`)](https://app.codecov.io/gh/apache/tinkerpop/commit/e86eed20a868d5ba8fcf64939f032b87093811c1?el=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) 75.16% compared to head [(`76df767`)](https://app.codecov.io/gh/apache/tinkerpop/pull/2424?src=pr=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) 71.23%. Additional details and impacted files ```diff @@ Coverage Diff @@ ## 3.6-dev#2424 +/- ## = - Coverage 75.16% 71.23% -3.93% = Files 1057 25-1032 Lines 63470 3772 -59698 Branches69360-6936 = - Hits 47706 2687 -45019 + Misses 13191 898 -12293 + Partials2573 187-2386 ``` [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/tinkerpop/pull/2424?src=pr=continue_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
