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

Fix unknown command throws System.InvalidOperationException: Enumeration already finished #542

Merged
merged 2 commits into from Feb 19, 2024

Conversation

sallerga
Copy link
Contributor

@sallerga sallerga commented Dec 4, 2023

#541

Do not call enumerator.Current if enumerator.MoveNext has not been called or returns false.

@@ -533,9 +535,10 @@ public CommandArgumentEnumerator(IEnumerator<CommandArgument> enumerator)

public bool MoveNext()
{
if (Current == null || !Current.MultipleValues)
if (!_currentValid || !Current.MultipleValues)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current is not nullable so I removed the null check. I think it was only used to check if MoveNext had been called. Not to allow Current to be null while iterating. Please let me know if I should add it again.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's also better because it not only checks that MoveNext has been called, but also that it returned true. The error was caused by calling Current after MoveNext has returned false.

Copy link
Contributor

@thomaslevesque thomaslevesque left a comment

Choose a reason for hiding this comment

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

LGTM. Better than my own attempt, actually, so I'll close my PR.

@@ -533,9 +535,10 @@ public CommandArgumentEnumerator(IEnumerator<CommandArgument> enumerator)

public bool MoveNext()
{
if (Current == null || !Current.MultipleValues)
if (!_currentValid || !Current.MultipleValues)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's also better because it not only checks that MoveNext has been called, but also that it returned true. The error was caused by calling Current after MoveNext has returned false.

@Gav-Brown
Copy link

Just to say thanks. Looking forward to this fix.

@prodigy
Copy link

prodigy commented Jan 10, 2024

Thank you for already fixing this, I'd have submitted a PR now otherwise. 🙏

@connorstorer-kbxcom
Copy link

FYI: I've got a work-around that's working for my scenario.

#541 (comment)

@paul-gauci
Copy link

I really need this fix :( . Can i do something to help?

@prodigy
Copy link

prodigy commented Feb 6, 2024

I really need this fix :( . Can i do something to help?

Just fork the repo, fix it there, and include the source in your solution.

And try this workaround:
#541 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants