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

Generic filters #27

Closed
petrsvihlik opened this issue Mar 20, 2017 · 5 comments
Closed

Generic filters #27

petrsvihlik opened this issue Mar 20, 2017 · 5 comments
Labels
enhancement groomed The issue has been groomed and should be in a good shape. help wanted

Comments

@petrsvihlik
Copy link
Contributor

petrsvihlik commented Mar 20, 2017

Current situation:
When I want to use e.g. GreaterThanFilter with DateTime I have to know how to format the DateTime so that the API accepts it.

Proposed solution:
We should solve this in a generic way. The filters should accept <T> as a value and handle the formatting internally based on the type.

  • public abstract class Filter (the base class for filters) should be switched into a generic class
  • it should have an extra constructor accepting <T> and storing it in a public property
  • getter of public string Value should be modified to take the type into consideration
    • for DateTime it should format it to the ISO 8601 format (accepted by the delivery API)
    • for the rest it should call just ToString()
  • adjust filters inheriting from Filter correspondingly (where it makes sense)
  • add unit tests
  • adjust the readme if necessary
  • all changes should preserve backward compatibility

Suggestions for any better approaches appreciated!

@JanLenoch
Copy link

As an alternative to the solution proposed by @petrsvihlik , it could be implemented in the following simple way:

  • The Filter class could have another constructor that takes object value instead of string value.
  • In the code of that constructor, there could be an if ... else that converts the filter value to a proper string representation, based on the type information.

Like so:

public Filter(string elementOrAttributePath, object value)
{
    ElementOrAttributePath = elementOrAttributePath;

    if (value != null)
    {
        if (value is DateTime?)
        {
            DateTime dtValue = (DateTime)value;
            Value = dtValue.ToString("yyyy-MM-dd");
        }
        else if (value is IEnumerable<string>)
        {
            IEnumerable<string> ieValue = (IEnumerable<string>)value;
            Value = string.Join(",", ieValue);
        }
        // else if ...
        else
        {
            Value = value.ToString();
        }
    }
}

Then, the signature of the existing constructors in covariant classes could be changed to accept object value (instead of string value), without losing backward compatibility.

Optional: We can enhance such code to also support our own Delivery SDK types (as mentioned in the table in https://forums.kenticocloud.com/discussion/31/strong-types-explained-code-generator/). Developers could then easily use e.g. the MultipleChoiceOption objects to pass as filtering values to the ContainsFilter.

@petrsvihlik
Copy link
Contributor Author

but we're open to any solutions...as long as they are backward compatible... :)

@alesk-kentico
Copy link
Contributor

I appreciate your effort, this issue has been mentioned several times and it should be resolved. However, we should not care about implementation yet, I consider API design and usage patterns more important. For Kentico developer the most important feature is to be able to easily discover what options are there. Let's admit it, IntelliSense is the primary way of finding out what the API can do.

The suggested approaches share the same weak spot – API does not tell you, which types of values can be used. Moreover, you can get wrong results when a value of unsupported type is converted to a string.

I propose a single solution – overloading methods. Just declare the relevant methods several times for each type that Delivery API supports. This way it will be very easy to see what types of parameters I can use and also it will be very difficult to make a mistake.

@Enngage
Copy link
Member

Enngage commented Jun 20, 2017

One possibility is to go even further and create some sort of 'query' builder where intellisense can also show you what filters you can use. This way you don't need to remember that you need to use 'ElementsParameter' or 'GreaterThenFilter' as you would just see all options available to you directly.

deliveryClient.items<SomeType>()
  .type('type')
  .equalsFilter('elements.name', 'value')
  .get();

It can be easily extended in future with other configurations/parameters. However, its obviously not backwards compatible and it would require major changes or a set of completely new methods.

This could be partially solved by implementing the IQueryable interface as mentioned here

@JanLenoch JanLenoch modified the milestone: Prioritization May 22, 2018
@JanLenoch JanLenoch added the groomed The issue has been groomed and should be in a good shape. label Aug 13, 2018
@JanLenoch
Copy link

This problem will be solved in a new issue: #119

@nkooman nkooman mentioned this issue Oct 17, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement groomed The issue has been groomed and should be in a good shape. help wanted
Projects
None yet
Development

No branches or pull requests

4 participants