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

WIP: Add expression macro for the selector type #1544

Closed
wants to merge 1 commit into from

Conversation

Danil-Grigorev
Copy link
Member

@Danil-Grigorev Danil-Grigorev commented Jul 21, 2024

Motivation

This is a follow-up on #1539 which adds a macro for Expression type, allowing to avoid unnecessary amount of into() calls while constructing a simple static expression.

Solution

Example:

expression!("foo" not in "bar", "baz");
// Instead of
Expression::NotIn("foo".into(), ["bar".into(), "baz".into()].into());

expression!("foo" in "bar","baz");
expression!("foo" == "bar");
let array = ["bar".into(), "baz".into()];
expression!("foo" in array);

To be done:

  • selector! macro (can be done?..)

Copy link

codecov bot commented Jul 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.7%. Comparing base (8329782) to head (8170848).
Report is 28 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1544   +/-   ##
=====================================
  Coverage   75.7%   75.7%           
=====================================
  Files         79      79           
  Lines       7252    7252           
=====================================
  Hits        5487    5487           
  Misses      1765    1765           
Files with missing lines Coverage Δ
kube-core/src/labels.rs 90.3% <ø> (ø)

@Danil-Grigorev Danil-Grigorev changed the title Add expression macro for the selector type WIP: Add expression macro for the selector type Jul 21, 2024
@SOF3
Copy link
Contributor

SOF3 commented Jul 22, 2024

Probably unpopular opinion, but doesn't this end up making users learn yet another foreign syntax of specifying labels? Right now there are already so many different syntaxes (kubectl CLI, LabelSelector matchLabels, LabelSelector matchExpressions), and we are adding yet another syntax here.

If the sole intention is to avoid the .into()s, can we make better Expression into-constructors instead? e.g.

pub struct In<KeyT, ValuesT>(KeyT, ValuesT);
impl<KeyT, ValuesT> From<In<KeyT, ValuesT>> for Expression
where
    KeyT: Into<String>,
    ValuesT: IntoIterator,
    <ValuesT as IntoIterator>::Item: Into<String>,
{ ... }

// vice versa for the other three variants

impl<T: Into<Expression>> From<T> for Selector { ... }

impl<T1, T2> From<(T1, T2)> for Selector
where
    T1: Into<Expression>,
    T2: Into<Expression>,
{ ... }

Then users can simply write

let selector: labels::Selector = (
    labels::In("foo", ["bar"]),
    labels::Equal("baz", "qux"),
).into();

@clux clux mentioned this pull request Jul 22, 2024
@clux
Copy link
Member

clux commented Jul 22, 2024

I have the same reservations as @SOF3 here (even though I did ask for this). I think it because is becoming more of a DSL than a typed convenience, when the primary aim is to get rid of all the into() pollution.

if the macro was a little more typed ala:

+selector::in!("foo", ["bar", "baz"]);
-expression!("foo" in "bar", "baz");

then i'd be less concerned. though, if SOF3's solution without macros work:

let selector: labels::Selector = (
    labels::In("foo", ["bar"]),
    labels::Equal("baz", "qux"),
).into();

then that ^ might be a better approach than macros.

@Danil-Grigorev
Copy link
Member Author

Yeah, I'm also seeing that this is the limit of macro usage, like selector! is not working for me with the current model. The idea was to give it a feel like writing a raw string (as before), but with typing:

selector!("foo" in ("bar", "baz"),"env" == "dev");

Macros are not giving readable compiler errors and I have the same feeling of a new DSL. Plus you can't easily track in which branch your expression ended up. Was a good exercise though 🙂

@Danil-Grigorev
Copy link
Member Author

Closing this to revise later.

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.

3 participants