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

renames skip -> ok in ProcessString #6064

Merged
merged 3 commits into from
May 3, 2022

Conversation

owen-d
Copy link
Member

@owen-d owen-d commented May 1, 2022

This confuses me every now and then, so I fixed the method var name and the last 2 references that used it as the name skip. Looking at how it's used, it should be named ok or keep or passes.

Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@owen-d owen-d requested a review from a team as a code owner May 1, 2022 22:58
@slim-bean
Copy link
Collaborator

What about 'matched' instead of 'ok' ?

@cyriltovena
Copy link
Contributor

I hear you but not sure ok does a better job no ? The point is that the line is filtered out.

@pull-request-size pull-request-size bot added size/L and removed size/S labels May 2, 2022
@owen-d
Copy link
Member Author

owen-d commented May 2, 2022

I was being lazy trying to avoid replacing all the other references which already use ok 😆 . Pushed an update to fix them 👍

Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
@owen-d owen-d force-pushed the logql-confusing-var-name branch from 267d114 to 0532f5c Compare May 2, 2022 12:43
…-name

Signed-off-by: Owen Diehl <ow.diehl@gmail.com>
Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

3 participants