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

feat: add support for ignore_case for in string array #44

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

gastonfournier
Copy link
Contributor

@gastonfournier gastonfournier commented Sep 20, 2023

  • Add support to a new operator following the same convention as other operators (equals_any_ignore_case)
  • Add test cases for different scenarios

None => true,
},
}
.invert(inverted)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is standard in rust, or if it's better to keep everything together. Lines started to grow big and this little change made it more readable, but I'm ok moving it back (also for consistency with the other operations)

Copy link
Member

@sighphyre sighphyre Sep 20, 2023

Choose a reason for hiding this comment

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

Unfortunately that's a logic change here, since this will also flip the state when the context value doesn't exist.

Annoying that neither the existing tests nor the client spec tests picked it up.

This might be more correct but I think it needs a bit more thought than a 5 minute glance

(But also, the code structure is fine, that looks like totally sane Rust to me)

@@ -8,7 +8,7 @@ Yggdrasil is a Rust project designed to create the core of the Unleash SDK domai

## Building the Core

Easy enough - run `cargo build` from the root of the project. You'll need an up to date set of Rust tools to do this.
Easy enough - run `cargo build` from the root of the project. You'll need an up to date set of Rust tools to do this (`rustup update stable`).
Copy link
Member

Choose a reason for hiding this comment

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

❤️

Copy link
Member

@sighphyre sighphyre left a comment

Choose a reason for hiding this comment

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

Absolutely amazing first dip into Ygg! Don't want to merge it yet but I think you've got the core ideas down just fine!

@gastonfournier gastonfournier changed the title feat: add support for ignore_case for in and not_in operators feat: add support for ignore_case for in string array Sep 21, 2023
@gastonfournier gastonfournier self-assigned this Sep 21, 2023
Copy link
Member

@sighphyre sighphyre left a comment

Choose a reason for hiding this comment

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

WHEE LGTM

@sighphyre
Copy link
Member

This isn't something we need in the grammar right now, so I'm going to let this hang for a bit. The point was a learning exercise

@sighphyre sighphyre marked this pull request as draft September 21, 2023 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants