apple / swift-algorithms

Commonly used sequence and collection algorithms for Swift
Apache License 2.0
5.97k stars 443 forks source link

Add BidirectionalCollection.trimming #4

Closed karwa closed 3 years ago

karwa commented 4 years ago

Description

Adds a '.trimming' method to BidirectionalCollection, returning a slice with matching elements removed from the start and end.

Detailed Design

extension BidirectionalCollection {

  /// Returns a `SubSequence` formed by discarding all elements at the start and end of the Collection
  /// which satisfy the given predicate.
  ///
  /// e.g. `[2, 10, 11, 15, 20, 21, 100].trimmimg(where: { $0.isMultiple(of: 2) })` == `[11, 15, 20, 21]`
  ///
  /// - parameters:
  ///    - predicate:  A closure which determines if the element should be omitted from the resulting slice.
  ///
  @inlinable
  public func trimming(where predicate: (Element) throws -> Bool) rethrows -> SubSequence
}

Documentation Plan

No existing guides need updating AFAIK. I haven't written a guide for this feature: is it necessary for every API to include this parallel documentation?

Test Plan

Tests are included.

Source Impact

Additive.

Checklist

karwa commented 4 years ago

In terms of code style, I think my defaults match this project (except that I use a 120 line length).

Does this project just use the default swift-format settings?

natecook1000 commented 4 years ago

Thanks for this, @karwa! 🎉

We do need a guide for this addition — the symbol docs describe the usage and semantics, while the guide can cover more about the intention, design, what choices were considered but not taken, and how this feature relates to other similar ones in other languages. A survey of the naming in other languages would be interesting to see — I think I've seen this called chop, chomp, and strip in addition to trim.

I've also been thinking a bit about how this relates to what we already have in the standard library. The matching existing method is drop(while:), which returns a subsequence without matching elements at the beginning. That method suffers from a couple different naming issues. First, when used with a trailing predicate, it's really unclear at the use site (e.g. array.drop { $0.isEven } looks like it would drop all the even numbers). Second, there isn't really an affordance for adding an analogous method that operates from the other end, dropping trailing elements that match a predicate.

All that makes me think that "trim" may be a better root verb for all three operations — maybe trimmingPrefix(where:), trimming(where:), and trimmingSuffix(where:)? What do you think?

karwa commented 4 years ago

@natecook1000 I've added a guide document. It may be a little verbose, I don't know 😅. That's just my style - it's how I write, but I did my best.

I agree with trim being a better root verb -- or trimmed as an adjective, which reads like "this is the collection, trimmed of these elements", with the verb form perhaps replacing/expanding on the mutating removeFirst/Last(Int) methods. I've mentioned the various methods in the standard library in the guide document. I tried not to get too bogged-down in that discussion, otherwise it'd turn more in to a swift-evolution pitch than a useful document for people using this library.

karwa commented 4 years ago

@natecook1000 is there anything remaining for me to do here?

karwa commented 4 years ago

Apologies for the delay. I've addressed your comments @xwu and @natecook1000 - documentation has been improved, and I think Xiaodi makes a good case for the name trimming. Code in the tests reads quite nicely after this, e.g.:

let results = [2, 10, 11, 15, 20, 21, 100].trimming { $0.isMultiple(of: 2) }