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

map and filter don't ensure compactness #60

Closed
mattbroussard opened this issue Oct 8, 2020 · 2 comments
Closed

map and filter don't ensure compactness #60

mattbroussard opened this issue Oct 8, 2020 · 2 comments

Comments

@mattbroussard
Copy link

In Designing the Delta Format, it says that:

we add the constraint that Deltas must be compact. With this constraint, the above representation is not a valid Delta, since it can be represented more compactly

However, when transforming a Delta using the map or filter methods, the result is not guaranteed to be compact since there can be adjacent ops that should be merged. A simple example case:

const input = new Delta()
  .insert('hello ')
  .insert('bold ', {bold: true})
  .insert('world');
const output = new Delta(input.filter(op => !op.attributes?.bold));

Expected:

{"ops":[{"insert":"hello world"}]}

Actual:

{"ops":[{"insert":"hello "},{"insert":"world"}]}

It appears that compose does enforce compactness, so you can use it to write a compactify function (this particular implementation assumes an insert-only/"document" delta, but could be easily extended to support the other op types):

function compactify(delta) {
  return delta.reduce((composed, op) =>
    composed.compose(
      new Delta()
        .retain(composed.length())
        .insert(op.insert, op.attributes)
    ),
    new Delta()
  );
}

However, it would be nice if one could assume the result of any method on Delta is also a valid Delta/list of ops. Perhaps since these op lists have to be put back through the Delta constructor to get a Delta object again, that would be an ideal place to put such validation/normalization

@jhchen
Copy link
Member

jhchen commented Oct 9, 2020

map() does not have to return ops at all so a compactness constraint on the output does not make sense. I would expect most people if given an array of n elements, they filter m elements, they expect the resulting array to be n-m elements. They would also expect the criteria they applied to filter not exist in the array afterwards. Compacting may violate both. So I'd place this in the being too helpful category.

@jhchen jhchen closed this as completed Oct 9, 2020
@mattbroussard
Copy link
Author

Fair enough re: the map and filter methods themselves; what do you think about doing compaction in the Delta constructor instead?

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

No branches or pull requests

2 participants