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 #4276: shouldn't drop all series when regex doesn't match #4336

Merged
merged 2 commits into from
Oct 6, 2015

Conversation

dgnorton
Copy link
Contributor

@dgnorton dgnorton commented Oct 6, 2015

No description provided.

@@ -2822,6 +2822,10 @@ func Walk(v Visitor, node Node) {
Walk(v, c)
}

case *DropSeriesStatement:
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're in here, should we add a panic for unsupported cases? Or can this be called for unsupported cases during normal operation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some statement types don't need to be represented as a case because they have no members that can be walked. If we added a panic then each of those would have to have an empty case. But, doing that still wouldn't prevent this type of bug because we still have to remember to update the Walk function if a walkable member is added to one of the types.

@otoolep
Copy link
Contributor

otoolep commented Oct 6, 2015

+1, tests look fine.

Of course, this was a pretty simple issue, I commented about the switch in case there is something we can do there.

dgnorton added a commit that referenced this pull request Oct 6, 2015
fix #4276: shouldn't drop all series when regex doesn't match
@dgnorton dgnorton merged commit a42fb78 into master Oct 6, 2015
@dgnorton dgnorton deleted the dgn-fix-4276 branch October 6, 2015 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants