snoyberg / mono-traversable

Type classes for mapping, folding, and traversing monomorphic containers
155 stars 64 forks source link

A few missing functions #139

Closed adnelson closed 6 years ago

adnelson commented 6 years ago

There are a couple of functions which I would love to have in this library, but I'm not sure what the correct classes to put them in would be:

I've made written generic versions of all of the above except split, but putting them in the appropriate type class(es) could make them more efficient. For example Text and ByteString have their own implementations of dropEnd which are undoubtedly more efficient than the one I have here.

-- | Remove `n` elements from the end of a sequence.
dropEnd :: (IsSequence s, SemiSequence s) => Index s -> s -> s
dropEnd n = reverse . drop n . reverse

-- | Add a prefix to a sequence, unless it already has it.
addPrefix :: (Eq (Element s), IsSequence s) => s -> s -> s
addPrefix p s = if isPrefixOf p s then s else p <> s

-- | Add a suffix to a sequence, unless it already has it.
addSuffix :: (Eq (Element s), IsSequence s) => s -> s -> s
addSuffix suf s = if isSuffixOf suf s then s else s <> suf

-- | Remove a prefix from a sequence, if it has it.
removePrefix :: (Index s ~ Int, Eq (Element s), IsSequence s) => s -> s -> s
removePrefix p s = if isPrefixOf p s then drop (length p) s else s

-- | Remove a suffix from a sequence, if it has it.
removeSuffix :: (Index s ~ Int, Eq (Element s), IsSequence s) => s -> s -> s
removeSuffix suf s = if isSuffixOf suf s then dropEnd (length suf) s else s

Admittedly the prefix/suffix stuff might be too specific to put into a generic library like this one, but I'm not sure. In any case I'd be happy to make a PR if that's desired, and get some guidance on what classes we'd want to put these in.

snoyberg commented 6 years ago

It looks like removePrefix and removeSuffix are slight specializations of stripPrefix and stripSuffix, e.g.:

removePrefix x y = fromMaybe y (stripPrefix x y)

I would find the presence of both versions of this function in the same library confusing personally. When I need the removePrefix functionality, I've always done it with fromMaybe.

As for addPrefix or addSuffix: I'd be OK adding them with a rename to ensurePrefix and ensureSuffix. The current name doesn't imply to me that it will check if the prefix/suffix is already present.

I think dropEnd can simply be added to IsSequence as you've defined it, and then provide overrides for more efficient behavior in some instances.

I'm not sure, but I believe the reason we don't have a split function right now is because that name means different things throughout the Haskell ecosystem. Is there a reason that splitWhen is insufficient?

adnelson commented 6 years ago

Thanks for the response! I learned a few things here.

cdepillabout commented 6 years ago

I've actually wanted removePrefix a few times myself.

removePrefix x y = fromMaybe y (stripPrefix x y)

It is simple enough to just use fromMaybe, but because you end up using the last argument (y) twice, it's not very convenient when writing in point-free style.

It'd be nice to use removePrefix when using the deriveJSON function from Aeson:

data Foo = Foo
  { fooA :: Int
  , fooB :: String
  }

$(deriveJSON defaultOptions { fieldLabelModifier = removePrefix "foo" } ''Foo)

Without removePrefix this code looks like the following:

$(deriveJSON defaultOptions { fieldLabelModifier = \s -> fromMaybe s (stripPrefix "foo" s) } ''Foo)

As an aside, defaultOptions and Options are defined as the following:

defaultOptions :: Options
defaultOptions =
  Options
    { fieldLabelModifier = id
    , ...
    }

data Options = Options
    { fieldLabelModifier :: String -> String
      -- ^ Function applied to field labels.
      -- Handy for removing common record prefixes for example.
    , ...
    }

Of course, removePrefix could be defined locally, but I think it would be a nice addition to Data.Sequences.

As long as the types are different for removePrefix and stripPrefix (and as long as there is a short example for each of them in the documentation), I don't think it would too confusing for end users.

snoyberg commented 6 years ago

I'm not too opinionated on it to be honest. I'd prefer a name that was more explicit about the difference, but if you send a PR, I'll accept even with the removePrefix name.

cdepillabout commented 6 years ago

@snoyberg I agree. The only thing I dislike about it as well is the similarity between the names stripPrefix and removePrefix.

If you (or anyone else) has an idea for a better name I'd be interested in hearing it.

Here's some similar names (but none of which I really like):

If I send a PR for this, should I also add a similar removeSuffix (or whatever it will be called) function as well?

snoyberg commented 6 years ago

I'd definitely go for including a *Suffix version at the same time. drop and delete both seem nice here, since both verbs have an existing meaning in the Haskell world of doing nothing if not possible (e.g., drop 5 [] == [], and Map.delete nonExistentKey m == m). I'd lean towards dropPrefix.

cdepillabout commented 6 years ago

Okay, I'll send a PR adding both dropPrefix and dropSuffix.