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

property: Add convenience overloads to isValid() #54

Open
jonaskello opened this issue Jan 30, 2023 · 3 comments · May be fixed by #55
Open

property: Add convenience overloads to isValid() #54

jonaskello opened this issue Jan 30, 2023 · 3 comments · May be fixed by #55
Assignees

Comments

@jonaskello
Copy link
Member

Currently we have this:

export function isValid(
  properties: PropertyValueSet.PropertyValueSet,
  filter: PropertyFilter,
  comparer: PropertyValue.Comparer = PropertyValue.defaultComparer
): boolean;

I propose that we add the possibility to pass unparsed strings to the parameters:

export function isValid(
  properties: string,
  filter: PropertyFilter,
  comparer: PropertyValue.Comparer = PropertyValue.defaultComparer
): boolean;
export function isValid(
  properties: PropertyValueSet.PropertyValueSet,
  filter: string,
  comparer: PropertyValue.Comparer = PropertyValue.defaultComparer
): boolean;
export function isValid(
  properties: string,
  filter: string,
  comparer: PropertyValue.Comparer = PropertyValue.defaultComparer
): boolean;

This could be done as overloads of the existing isValid() function without a breaking change. Not that I in particular like overloading, but the alternative would be to have 4 separate named versions. I find it hard to come up with good and short names for all 4.

@jonaskello jonaskello changed the title property: Add convince overloads to isValid() property: Add convenience overloads to isValid() Jan 30, 2023
@jonaskello jonaskello self-assigned this Jan 30, 2023
@jonaskello
Copy link
Member Author

Actually overloading is not needed, we can just do union types instead:

export function isValid(
  properties: PropertyValueSet.PropertyValueSet | string,
  filter: PropertyFilter | string,
  comparer: PropertyValue.Comparer = PropertyValue.defaultComparer
): boolean

@jonaskello
Copy link
Member Author

jonaskello commented Jan 30, 2023

Actually it cannot be done by neither union types nor overload since when we have raw strings there is a need for a UnitLookup function, but when we get parsed values (as in the current version) this is not needed. This means we would have varying number of parameters in the overloads which AFAIK is not supported by typescript.

UPDATE: It is possible to have different number of params in the overloads as long as the implementation supports all permutations. However this is a bit messy to implement because of the optional comparer param. But it should be possible.

@jonaskello jonaskello linked a pull request Jan 30, 2023 that will close this issue
@jonaskello
Copy link
Member Author

One issue with the overloads is that since they accept a string, the parsing of that string may fail. It then becomes a question how to handle the failures. The two main paths are :

  • Built-in behaviour, like throwing an exception, or just defaulting to an empty PVS and/or PF
  • Returning a result to let the application handle the parse failure

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 a pull request may close this issue.

1 participant