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

 ## 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

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


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

-    => 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

            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

-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

-                        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

	-- 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

Uh oh!

Conversation

danbroooks commented Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parsonsmatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbroooks Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbroooks commented Apr 20, 2021

Uh oh!

parsonsmatt commented Apr 20, 2021

Uh oh!

danbroooks commented Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

danbroooks commented Apr 20, 2021 •

edited

Loading

danbroooks Apr 20, 2021 •

edited

Loading

danbroooks commented Apr 20, 2021 •

edited

Loading