Skip to content

Resolve filter parsing issues#4760

Merged
stevenaw merged 4 commits intonunit:mainfrom
stevenaw:4651-fix-filter-parsing
Jul 16, 2024
Merged

Resolve filter parsing issues#4760
stevenaw merged 4 commits intonunit:mainfrom
stevenaw:4651-fix-filter-parsing

Conversation

@stevenaw
Copy link
Member

@stevenaw stevenaw commented Jul 15, 2024

Fixes #4651
Fixes #4589

First commit is just to establish a baseline failing CI run with the two known scenarios. Second commit fixes the underlying issue.

@stevenaw stevenaw marked this pull request as draft July 15, 2024 02:45
@stevenaw stevenaw force-pushed the 4651-fix-filter-parsing branch from fe1cd68 to 9840405 Compare July 16, 2024 02:24
@stevenaw stevenaw changed the title WIP: Resolve filter parsing issues Resolve filter parsing issues Jul 16, 2024
@stevenaw stevenaw marked this pull request as ready for review July 16, 2024 03:00
@stevenaw
Copy link
Member Author

This is ready for review

Copy link
Member

@manfred-brands manfred-brands left a comment

Choose a reason for hiding this comment

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

I have some questions

{
if (reader.Depth < parents.Count)
{
parents.Pop();
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that the code that 'Pops' in lines 153-157 can be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! I recall that code being hit, but it was before having added my own changes. I'll try and check this out tonight to confirm if it can be removed.

stevenaw and others added 2 commits July 16, 2024 06:54
Copy link
Member

@manfred-brands manfred-brands left a comment

Choose a reason for hiding this comment

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

Looks good now.

@stevenaw
Copy link
Member Author

Thanks @manfred-brands !

@stevenaw stevenaw merged commit 0bb5099 into nunit:main Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants