Simplify Line type in Quasi module, always use NonEmpty #1231

danbroooks · 2021-04-20T18:12:32Z

Before submitting your PR, check that you've:

Documented new APIs with Haddock markup
Added @since declarations to the Haddock
Ran stylish-haskell on any changed files.
Adhered to the code style (see the .editorconfig file for details)

After submitting your PR:

Update the Changelog.md file with a link to your PR
Bumped the version number if there isn't an (unreleased) on the Changelog
Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

This is a followon from #1206 that addresses the TODO comment here:

persistent/persistent/Database/Persist/Quasi.hs

Lines 546 to 555 in 53359c8

    
           -- TODO: refactor to return (Line' NonEmpty), made possible by 
        
           -- https://github.com/yesodweb/persistent/pull/1206 but left out 
        
           -- in order to minimize the diff 
        
           parseLine :: Text -> Maybe Line 
        
           parseLine txt = 
        
               case tokenize txt of 
        
                 [] -> 
        
                     Nothing 
        
                 toks -> 
        
                     pure $ Line (parseIndentationAmount txt) toks

Following recent refactoring there is not actually any instance of Line where the contents of the list are not NonEmpty, so this PR changes that type to always use NonEmpty, rather than being polymorphic, which greatly simplifies a lot of the code around Line, and means we can also get rid of the skipEmpty function, as NonEmpty guarantees us nonempty-ness 👍

parsonsmatt

looks great!

made a note that the version is going to be 2.11.1.1, which i'm planning on releasing today

parsonsmatt · 2021-04-20T19:14:49Z

persistent/ChangeLog.md

@@ -2,6 +2,8 @@

 ## 2.12.1.0

+* [#1231](https://github.com/yesodweb/persistent/pull/1231)
+    * Simplify Line type in Quasi module, always use NonEmpty 


2.12.1.0 has already been released, this is going to get bundled in with 2.12.1.1

No probs I will shift this up 👍

parsonsmatt · 2021-04-20T19:15:35Z

persistent/Database/Persist/Quasi.hs

-          pure $ Line (parseIndentationAmount txt) toks
+parseLine txt = do
+    parsedTokens <- NEL.nonEmpty (tokenize txt)
+    pure $ Line (parseIndentationAmount txt) parsedTokens


parsonsmatt · 2021-04-20T19:16:43Z

persistent/Database/Persist/Quasi.hs

 lowestIndent
    :: Functor f
    => Foldable f
-    => Functor g
-    => f (Line' g)
+    => f Line
    -> Int
 lowestIndent = minimum . fmap lineIndent


Can we specialize this function, too? I feel like we can get away with specializing this, possibly even to NonEmpty which would make it even safer (mimumum [] = undefined).

parsonsmatt · 2021-04-20T19:18:37Z

persistent/test/main.hs

@@ -212,7 +214,7 @@ main = hspec $ do
        it "mid-token quote in later token" $
            parseLine "foo bar baz=(bin\")" `shouldBe`
                Just
-                    ( Line 0
+                    ( Line 0 $ nonEmptyOrFail


For the test suite, we could enable OverloadedLists, and then we can use list literal syntax for NonEmpty a - it's a gross kludge to write [] :: NonEmpty Int, but, lmao, it does make the test syntax nicer.

parsonsmatt · 2021-04-20T19:21:22Z

persistent/test/main.hs

-asTokens :: [T.Text] -> [Token]
-asTokens = fmap Token
+nonEmptyOrFail :: [a] -> NonEmpty a
+nonEmptyOrFail = maybe failure id . NEL.nonEmpty


there's the horribly namedNEL.fromList :: [a] -> NonEmpty a unless by some act of grace it was removed

buuut if we just use list literal syyntax I think this can go away

danbroooks · 2021-04-20T21:12:55Z

persistent/test/main.hs

-                        Just
-                            [ Line 0 [Token "Foo"]
-                            , Line 0 [DocComment "Hello"]
-                            ]


The switch to using OverloadedLists caused this to give an ambiguous type error:

• Couldn't match expected type ‘GHC.Exts.Item (t0 Line)’ with actual type ‘Line’ The type variable ‘t0’ is ambiguous • In the expression: Line 0 [Token "Foo"] In the first argument of ‘Just’, namely ‘[Line 0 [Token "Foo"], Line 0 [DocComment "Hello"]]’ In the second argument of ‘shouldBe’, namely ‘Just [Line 0 [Token "Foo"], Line 0 [DocComment "Hello"]]’ | 236 | [ Line 0 [Token "Foo"] | ^^^^^^^^^^^^^^^^^^^^

I've opted to actually just delete this test, this function is being tested already with the other tests here (the one above and below it), and also in the context of multiple lines, in tests like this one:

persistent/persistent/test/main.hs

Lines 538 to 547 in 813fe42

it "recognizes comments" $ do

let text = "Foo\n x X\n-- | Hello\nBar\n name String"

let expected =

Line { lineIndent = 0, tokens = asTokens ["Foo"] } :|

[ Line { lineIndent = 2, tokens = asTokens ["x", "X"] }

, Line { lineIndent = 0, tokens = [DocComment "Hello"] }

, Line { lineIndent = 0, tokens = asTokens ["Bar"] }

, Line { lineIndent = 1, tokens = asTokens ["name", "String"] }

]

preparse text `shouldBe` Just expected

Cool works for me!

danbroooks · 2021-04-20T21:30:47Z

Hey I wasn't finished! 😂 sorry, maybe I shouldn't have pushed that stuff up. I will have another PR shortly, it should still fit in fine as a follow on I think :)

parsonsmatt · 2021-04-20T21:33:31Z

Oh, shoot!

I've already released as 2.11.1.1 😅

Try to mark a PR as a draft if it's not ready yet?

danbroooks · 2021-04-20T21:34:56Z

Yeah its no big deal, what I meant was I wasn't done with the review updates, I've still yet to remove the nonEmptyOrFail function, this PR is totally fine to be a part of that release 👍

Simplify Line type

da25517

danbroooks changed the title ~~Simplify Line type, always use NonEmpty~~ Simplify Line type in Quasi module, always use NonEmpty Apr 20, 2021

Add to changelog

563b435

parsonsmatt approved these changes Apr 20, 2021

View reviewed changes

danbroooks added 4 commits April 20, 2021 21:47

Run stylish haskell

e2deb1b

Update changelog

7a6948a

Post-review tweaks

01dfe4c

Merge branch 'master' into simplify-line

b128dc5

danbroooks commented Apr 20, 2021

View reviewed changes

parsonsmatt approved these changes Apr 20, 2021

View reviewed changes

parsonsmatt merged commit 7bb6abf into yesodweb:master Apr 20, 2021

danbroooks mentioned this pull request Apr 20, 2021

Remove nonEmptyOrFail function from recent tests #1237

Merged

7 tasks

danbroooks deleted the simplify-line branch April 27, 2021 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify Line type in Quasi module, always use NonEmpty #1231

Simplify Line type in Quasi module, always use NonEmpty #1231

danbroooks commented Apr 20, 2021 •

edited

Loading

parsonsmatt left a comment

parsonsmatt Apr 20, 2021

danbroooks Apr 20, 2021

parsonsmatt Apr 20, 2021

parsonsmatt Apr 20, 2021

parsonsmatt Apr 20, 2021

parsonsmatt Apr 20, 2021

parsonsmatt Apr 20, 2021

danbroooks Apr 20, 2021 •

edited

Loading

parsonsmatt Apr 20, 2021

danbroooks commented Apr 20, 2021

parsonsmatt commented Apr 20, 2021

danbroooks commented Apr 20, 2021 •

edited

Loading

	-- TODO: refactor to return (Line' NonEmpty), made possible by
	-- https://github.com/yesodweb/persistent/pull/1206 but left out
	-- in order to minimize the diff
	parseLine :: Text -> Maybe Line
	parseLine txt =
	case tokenize txt of
	[] ->
	Nothing
	toks ->
	pure $ Line (parseIndentationAmount txt) toks

	it "recognizes comments" $ do
	let text = "Foo\n x X\n-- \| Hello\nBar\n name String"
	let expected =
	Line { lineIndent = 0, tokens = asTokens ["Foo"] } :\|
	[ Line { lineIndent = 2, tokens = asTokens ["x", "X"] }
	, Line { lineIndent = 0, tokens = [DocComment "Hello"] }
	, Line { lineIndent = 0, tokens = asTokens ["Bar"] }
	, Line { lineIndent = 1, tokens = asTokens ["name", "String"] }
	]
	preparse text `shouldBe` Just expected

Simplify Line type in Quasi module, always use NonEmpty #1231

Simplify Line type in Quasi module, always use NonEmpty #1231

Conversation

danbroooks commented Apr 20, 2021 • edited Loading

parsonsmatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbroooks Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbroooks commented Apr 20, 2021

parsonsmatt commented Apr 20, 2021

danbroooks commented Apr 20, 2021 • edited Loading

danbroooks commented Apr 20, 2021 •

edited

Loading

danbroooks Apr 20, 2021 •

edited

Loading

danbroooks commented Apr 20, 2021 •

edited

Loading