fix: Codec for article slugs #128
Conversation
src/Data/Slug.purs
Outdated
| -- | A `Slug` represents a string value which is guaranteed to have the | ||
| -- | following qualities: | ||
| -- | | ||
| -- | - it is not empty | ||
| -- | - it consists of groups of characters separated by `-` dashes, | ||
| -- | where the slug cannot begin or end with a dash, and there can | ||
| -- | never be two dashes in a row. | ||
| -- | | ||
| -- | Example: `Slug "this-is-an-article-slug"` | ||
| newtype Slug = Slug String |
There was a problem hiding this comment.
This was copied from https://github.com/thomashoneyman/purescript-slug/blob/0142716b49758829d5b48244fe1e9f87ea935b77/src/Slug.purs#L21-L31, removing the following guarantees: "every character with a defined notion of case is lower-cased" and "it consists of alphanumeric groups of characters".
| -- Replace non-Latin 1 characters with spaces to be stripped later. | ||
| onlyAlphaNum = | ||
| fromCodePointArray | ||
| <<< map (\x -> if isLatin1 x then x else codePointFromChar ' ') |
There was a problem hiding this comment.
Copied from https://github.com/thomashoneyman/purescript-slug/blob/0142716b49758829d5b48244fe1e9f87ea935b77/src/Slug.purs#L75, removing isAlphaNum x
| -- | ``` | ||
| generate :: String -> Maybe Slug | ||
| generate s = do | ||
| let arr = words $ onlyLatin1 $ stripApostrophes s |
There was a problem hiding this comment.
Copied from https://github.com/thomashoneyman/purescript-slug/blob/0142716b49758829d5b48244fe1e9f87ea935b77/src/Slug.purs#L63 removing String.toLower
Allow for non-alphanumeric and non-lower-case characters in slugs. Fixes thomashoneyman#127.
src/Data/Slug.purs
Outdated
| derive newtype instance Semigroup Slug | ||
|
|
||
| instance Show Slug where | ||
| show (Slug str) = "(Slug " <> str <> ")" |
There was a problem hiding this comment.
I think we should ditch these instances since they aren’t used here (I generally don’t use Show, and we use codecs instead of JSON instances)
There was a problem hiding this comment.
I removed the *Json, Show, and Semigroup instances (leaving Eq and Ord since those are used in instance ordRoute :: Ord Route).
src/Data/Slug.purs
Outdated
| truncate n (Slug s) | ||
| | n < 1 = Nothing | ||
| | n >= String.length s = Just (Slug s) | ||
| | otherwise = generate $ String.take n s |
There was a problem hiding this comment.
| | otherwise = generate $ String.take n s | |
| | otherwise = generate $ String.take n s | |
|
Thanks! I’m fine integrating this module into the code base to fix this issue. Looks like we should also remove the import from Spago? |
Will do 👍 I'm surprised there's no warning from |
|
Also, I see when running Edit: I added it here ced03b5 but I can remove that commit if we don't want to track |
Removing unused `Semigroup`, `Show`, `EncodeJson`, and `DecodeJson` instances from `Slug` module, adding newline at end of file, and removing unused `slug` dependency from `spago.yaml`.
I created an issue in |
|
We can use the —pedantic-packages flag to get warnings for unused dependencies. And yes, the lock being ignored is no longer relevant, we do want the spago lock committed! |
|
I merged this because it fixes the existing issue — but if you have follow up things you’d like to add I’m all ears! |
Allow for non-alphanumeric and non-lower-case characters in slugs.
Fixes #127.