Provide context parsing in the `mkMessage` function #283

Oleksandr-Zhabenko · 2024-08-15T12:19:21Z

Provided context parsing in the mkMessage function. Resolves issue #282: mkMessage should support context parsing

CI passed successfully. See the link below:
https://github.com/Oleksandr-Zhabenko/shakespeare/actions/runs/10403325875

jezen · 2024-08-15T12:54:32Z

.github/workflows/tests.yml

+# Is taken from the https://github.com/yesodweb/yesod/blob/master/.github/workflows/tests.yml template
+


This comment probably isn't necessary.

jezen · 2024-08-15T12:55:36Z

Text/Shakespeare/I18N.hs

@@ -62,22 +63,22 @@ module Text.Shakespeare.I18N
    ) where

 import Language.Haskell.TH.Syntax hiding (makeRelativeToProject)
-import Control.Applicative ((<$>))
+--import Control.Applicative ((<$>))


If this import isn't necessary, it's better just to delete the line.

jezen · 2024-08-15T12:55:54Z

Text/Shakespeare/I18N.hs

 import Control.Arrow ((***))
-import Data.Monoid (mempty, mappend)
+--import Data.Monoid (mempty, mappend)


We can delete this line also.

jezen · 2024-08-15T12:56:38Z

Text/Shakespeare/I18N.hs

+
+                 parseName = do
+                      cxt' <- option [] parseContext
+ --                     name' <- parseWord    -- This one is no longer needed as a separate argument


Let's just keep the code that we need 🙂

Ok. I will tidy up the source files.

jezen

Thanks for tidying up. Now we just need to demonstrate that this is not a breaking change, and that it produces correct code for the scenario documented in the associated issue (and we should squash before merging).

This reverts commit 4fcf511, reversing changes made to 6ca155b.

jezen · 2024-08-23T10:26:00Z

ChangeLog.md

-### 2.1.1
-
-* Add support for `TypeApplications` inside Shakespeare quasiquotes
-


Is there a good reason why this changelog entry was removed? This looks like a mistake.

On Hackage the version 2.1.1 is the last one and is already deprecated.
https://hackage.haskell.org/package/shakespeare-2.1.1

Ok, but it shouldn't be removed even if it's deprecated. The deprecated version should continue to exist in the version history.

jezen · 2024-08-23T10:27:17Z

Text/Shakespeare/I18N.hs

@@ -255,13 +301,13 @@ conP name = ConP name []
 conP = ConP
 #endif

-toCon :: String -> SDef -> Con
+toCon :: String -> SDef -> Con  


Let's not introduce trailing whitespace.

jezen · 2024-08-23T10:27:38Z

shakespeare.cabal

-version:         2.1.1
+version:         2.1.0


I would expect this number to go up, not down.

I think it can be changed to 2.1.2

jezen · 2024-08-23T10:28:32Z

test/Text/Shakespeare/BaseSpec.hs

-  it "parseDeref parse single type applications" $ do
-    runParser parseDeref () "" "x @y" `shouldBe`
-      Right
-        (DerefBranch
-          (DerefIdent (Ident "x"))
-          (DerefType "y"))
-  it "parseDeref parse unit type applications" $ do
-    runParser parseDeref () "" "x @()" `shouldBe`
-      Right
-        (DerefBranch
-          (DerefIdent (Ident "x"))
-          (DerefType "()"))
-  it "parseDeref parse compound type applications" $ do
-    runParser parseDeref () "" "x @(Maybe String)" `shouldBe`
-      Right
-        (DerefBranch
-          (DerefIdent (Ident "x"))
-          (DerefType "Maybe String"))
-  it "parseDeref parse single @ as operator" $ do
-    runParser parseDeref () "" "x @ y" `shouldBe`
-      Right
-        (DerefBranch
-          (DerefBranch (DerefIdent (Ident "@")) (DerefIdent (Ident "x")))
-          (DerefIdent (Ident "y")))


Why were these tests removed?

It is strange. I have not edited the file. Should I add these lines into the BaseSpec.hs?

Looks like a minor git accident then 🙂 You can just revert this change.

jezen · 2024-08-23T10:29:15Z

Text/Shakespeare/I18N.hs

+            cxt  -- Here the parsed context should be added, otherwise []
+            (ConT ''RenderMessage `AppT` (if ' ' `elem` master' then let (ts, us) = break (== ' ') . filter (\x -> x /= '(' && x /= ')') $ master' in let us1 = filter (/= ' ') us in ParensT (ConT (mkName ts) `AppT` VarT (mkName us1))  else ConT $ mkName master') `AppT` ConT mname)


Is there a nicer way to format this? I would prefer to see lines limited to 80 columns.

jezen · 2024-08-23T10:30:12Z

Text/Shakespeare/I18N.hs

            [ FunD (mkName "renderMessage") $ c1 ++ c2 ++ [c3]
            ]
        ]
-
-toClauses :: String -> String -> (Lang, [Def]) -> Q [Clause]
+           where (dt1, cxt0) = case (parse parseName "" dt) of


Let's improve the formatting of all the new parsing code here.

…ssue Added some code improvements

Oleksandr-Zhabenko changed the title ~~Introduced contexts parsing in the mkMessage function. Resolves issue #282: mkMessage should support context parsing~~ Provided context parsing in the mkMessage function. Resolves issue #282: mkMessage should support context parsing Aug 15, 2024

jezen changed the title ~~Provided context parsing in the mkMessage function. Resolves issue #282: mkMessage should support context parsing~~ Provide context parsing in the mkMessage function Aug 15, 2024

jezen reviewed Aug 15, 2024

View reviewed changes

jezen requested changes Aug 15, 2024

View reviewed changes

Revert "Merge pull request #279 from mixphix/splice-tyapps"

59ca619

This reverts commit 4fcf511, reversing changes made to 6ca155b.

jezen reviewed Aug 23, 2024

View reviewed changes

Added code to resolve mkMessage should support context parsing #282 i…

4771c04

…ssue Added some code improvements

Oleksandr-Zhabenko closed this Aug 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide context parsing in the `mkMessage` function #283

Provide context parsing in the `mkMessage` function #283

Oleksandr-Zhabenko commented Aug 15, 2024 •

edited

Loading

jezen Aug 15, 2024

jezen Aug 15, 2024

jezen Aug 15, 2024

jezen Aug 15, 2024

Oleksandr-Zhabenko Aug 15, 2024 •

edited

Loading

jezen left a comment

jezen Aug 23, 2024

Oleksandr-Zhabenko Aug 23, 2024

jezen Aug 23, 2024

jezen Aug 23, 2024

jezen Aug 23, 2024

Oleksandr-Zhabenko Aug 23, 2024

jezen Aug 23, 2024

Oleksandr-Zhabenko Aug 23, 2024 •

edited

Loading

jezen Aug 23, 2024

jezen Aug 23, 2024

jezen Aug 23, 2024

		# Is taken from the https://github.com/yesodweb/yesod/blob/master/.github/workflows/tests.yml template

		### 2.1.1

		* Add support for `TypeApplications` inside Shakespeare quasiquotes

		cxt -- Here the parsed context should be added, otherwise []
		(ConT ''RenderMessage `AppT` (if ' ' `elem` master' then let (ts, us) = break (== ' ') . filter (\x -> x /= '(' && x /= ')') $ master' in let us1 = filter (/= ' ') us in ParensT (ConT (mkName ts) `AppT` VarT (mkName us1)) else ConT $ mkName master') `AppT` ConT mname)

		version: 2.1.1
		version: 2.1.0

Provide context parsing in the mkMessage function #283

Provide context parsing in the mkMessage function #283

Conversation

Oleksandr-Zhabenko commented Aug 15, 2024 • edited Loading

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

Oleksandr-Zhabenko Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

jezen 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

Oleksandr-Zhabenko Aug 23, 2024 • edited Loading

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

Provide context parsing in the `mkMessage` function #283

Provide context parsing in the `mkMessage` function #283

Oleksandr-Zhabenko commented Aug 15, 2024 •

edited

Loading

Oleksandr-Zhabenko Aug 15, 2024 •

edited

Loading

Oleksandr-Zhabenko Aug 23, 2024 •

edited

Loading