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.

Sign up for GitHub

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

Jump to bottom

maint: convert hardcoded operators to constants #813

Merged
merged 4 commits into from
Aug 1, 2023
Merged

maint: convert hardcoded operators to constants #813

merged 4 commits into from
Aug 1, 2023

Conversation

kentquirk
Copy link
Contributor

@kentquirk kentquirk commented Jul 31, 2023 •
edited
Loading

Which problem is this PR solving?

  • This was intended to be purely a refactoring to pull out constant values
  • But it uncovered a bug -- we had a switch where we weren't checking the ">=" case!

Short description of the changes

  • Constantize all the things
  • Add the missing switch case
  • Write some tests (not exhaustive but try to prove all the major spots work)
  • Fix the boolean bug that was also uncovered by the tests

@kentquirk kentquirk requested a review from a team as a code owner July 31, 2023 19:14
TylerHelmuth
TylerHelmuth reviewed Jul 31, 2023
View reviewed changes
Copy link
Contributor

@TylerHelmuth TylerHelmuth left a comment •
edited
Loading

Choose a reason for hiding this comment

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

With the new case I expected a new test. Did we have a test already that was passing when it should not have?

@kentquirk kentquirk requested a review from fchikwekwe July 31, 2023 23:39
@kentquirk kentquirk mentioned this pull request Aug 1, 2023
Merged
fchikwekwe
fchikwekwe approved these changes Aug 1, 2023
View reviewed changes
Copy link
Contributor

@fchikwekwe fchikwekwe left a comment

Choose a reason for hiding this comment

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

Great work, this actually closes #592 as well so great job.

@fchikwekwe
Copy link
Contributor

Oh, duh. I just now saw the reference to that issue in your branch name now.

@kentquirk kentquirk merged commit f67fa44 into main Aug 1, 2023
@kentquirk kentquirk deleted the kent.592 branch August 1, 2023 19:42
kentquirk added a commit that referenced this pull request Aug 1, 2023
@kentquirk
feat: Add has-root-span operator to rules (#814)
66dc60b 
Dependent on #813; that should be merged before this one.


## Which problem is this PR solving?

- Fixes #610 

## Short description of the changes

- Adds a new rule operator, `has-root-span`, that evaluates to a boolean
based on whether the root span for the trace has arrived when the rules
are evaluated.
- Add tests for it
- Update metadata (but does not regenerate the docs yet; that will come
before the release)
@kentquirk kentquirk mentioned this pull request Aug 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TylerHelmuth TylerHelmuth TylerHelmuth left review comments

@fchikwekwe fchikwekwe fchikwekwe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kentquirk @fchikwekwe @TylerHelmuth