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

Implement starts_with, ends_with, contains, like, not_like operations on StringFilter and add is_not_null operation #116

Closed
wants to merge 4 commits into from

Conversation

skopz356
Copy link
Contributor

@skopz356 skopz356 commented Jan 17, 2023

Implement starts_with, ends_with, contains, like, not_like operations on StringFilter and add is_not_null operation

PR Info

New Features

  • This pull request implements starts_with, ends_with, contains, like, not_like operations and also adds is_not_null. These operations were in api. But they were not implemented.

Changes

  • Adds is_not_null operation to TypeFilter and StringFilter. So now developers can also use this operation.

Comment on lines 137 to +154
fn contains(&self) -> Option<String> {
panic!("FilterType does not support contains")
None
}

fn starts_with(&self) -> Option<String> {
panic!("FilterType does not support starts_with")
None
}

fn ends_with(&self) -> Option<String> {
panic!("FilterType does not support ends_with")
None
}

fn like(&self) -> Option<String> {
panic!("FilterType does not support like")
None
}

fn not_like(&self) -> Option<String> {
panic!("FilterType does not support not_like")
None
Copy link
Member

Choose a reason for hiding this comment

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

Design choice: no-operation or a explicit panic.

I second the no-operation as changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so if I understand you correctly that means that you prefer no-operation and I should keep these changes? 😄

Comment on lines 87 to +88
pub is_null: Option<bool>,
pub is_not_null: Option<bool>,
Copy link
Member

Choose a reason for hiding this comment

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

A strange question. What if one supply false for these two fields?

{
  customer(
    filters: { active: {
      # Normally...
      is_null: true,
      is_not_null: true,
      # How about...?
      is_null: false,
      is_not_null: false,
    } }
  ) {
    ...
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed.

Comment on lines 199 to 207
condition = condition.add(Column::#column_enum_name.is_null())
}
}

if let Some(is_not_null_value) = seaography::FilterTrait::is_not_null(#column_name) {
if is_not_null_value {
condition = condition.add(Column::#column_enum_name.is_not_null())
}
}
Copy link
Member

Choose a reason for hiding this comment

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

If one can supply a false value to is_null and is_not_null. Then, we better handle it properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@billy1624
Copy link
Member

Hey @skopz356, thanks again for contributing!! Sorry for the delay

@skopz356
Copy link
Contributor Author

@billy1624 Thank you for your review! I will look at your suggestions. I have a question I also thought about implementing these methods (contains, like, etc) on integer and float types. What is your opinion on that? Should it be there or not

@billy1624
Copy link
Member

@billy1624 Thank you for your review! I will look at your suggestions. I have a question I also thought about implementing these methods (contains, like, etc) on integer and float types. What is your opinion on that? Should it be there or not

Hey @skopz356, sorry for the delay. I think the contain and like method is only applicable to String types. So, no, I don't think we should implement those methods for the integer and float types.

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

Successfully merging this pull request may close these issues.

StringFilter should support LIKE
2 participants