Maintenance: linting

[Magellol]

Invoke the linting too through intlc-internal

A "faulty" json file can be found here https://github.com/unsplash/unsplash-web/blob/master/app/routes/Photos/lang/en-US.translations.json

A good one here: https://github.com/unsplash/unsplash-web/blob/master/app/routes/Press/lang/en-US.translations.json

e.g

intlc-internal lint app/routes/Photos/lang/en-US.translations.json
TooManyInterpolations
# exit 1

intlc-internal lint app/routes/Press/lang/en-US.translations.json
Success
# exit 0

TODO

• Make linting fail on Failure msg

Nice to have

• Accumulate linting errors. Right now it will stop as soon as it sees one

[samhh]

I count three pattern matches with case that could be inlined into their functions. 😛 e.g.

-- before
f x = case x of y -> z
-- or
f = \case y -> z

-- after
f y = z

[Magellol]

I've found these two helpful to change 5cc9666

I'm guessing the third one is count = \case although if the equivalent would be

count String = 0
count Number = 0
count (Bool {}) = 1

Would this be very similar to using point-free case here? Or perhaps there is another equivalent I'm not seeing 👍

[samhh]

Yep that's the one, and yeah it's very similar, just a case of common idioms. 🙂

[Magellol]

@samhh I've got a start on invoking the Linting module in the CLI af9013c — I think it might be worth looking at accumulating errors already? 🤔 (I wonder if we could print lines and columns from source as well)

[samhh]

I think it might be worth looking at accumulating errors already?

Certainly! It'd be great to get output something like this:

"msgA": too many interpolations
"msgB": non-ASCII, too many interpolations

I wonder if we could print lines and columns from source as well

This needs some research. At the moment our AST doesn't hold onto source information after parsing is complete. As with nicer parser errors this could come later, it's not a blocker.

[samhh]

Nearly there, however I don't think this handles (pre-)nested interpolations yet? In the following f will be correctly flagged however g should be too and isn't:

{
  "f": { "message": "{x, select, xa {}} {y, select, ya {}}" },
  "g": { "message": "{x, select, xa {{y, select, ya {}}}}" }
}

I think this test catches it:

diff --git a/test/Intlc/LinterSpec.hs b/test/Intlc/LinterSpec.hs
index 6eb1541..f63276e 100644
--- a/test/Intlc/LinterSpec.hs
+++ b/test/Intlc/LinterSpec.hs
@@ -28,3 +28,5 @@ spec = describe "linter" $ do
   it "does not lint dynamic streams with 2 or more complex interpolations" $ do
     lint (Dynamic (fromList [Interpolation (Arg "Hello" (Callback [])), Interpolation (Arg "Hello" (Bool [] []))])) `shouldBe` Failure TooManyInterpolations
 
+  it "does not lint nested streams" $ do
+    lint (Dynamic (fromList [Interpolation (Arg "outer" (Callback [Interpolation (Arg "inner" (Callback []))]))])) `shouldBe` Failure TooManyInterpolations

As an aside because errors in Haskell are lazy you can cheaply validate that we stop iterating like so (could also use undefined):

diff --git a/test/Intlc/LinterSpec.hs b/test/Intlc/LinterSpec.hs
index 6eb1541..9ba075f 100644
--- a/test/Intlc/LinterSpec.hs
+++ b/test/Intlc/LinterSpec.hs
@@ -28,3 +28,5 @@ spec = describe "linter" $ do
   it "does not lint dynamic streams with 2 or more complex interpolations" $ do
     lint (Dynamic (fromList [Interpolation (Arg "Hello" (Callback [])), Interpolation (Arg "Hello" (Bool [] []))])) `shouldBe` Failure TooManyInterpolations
 
+  it "stops iterating after second stream" $ do
+    lint (Dynamic (fromList [Interpolation (Arg "Hello" (Callback [])), Interpolation (Arg "Hello" (Bool [] [])), error "should not reach here"])) `shouldBe` Failure TooManyInterpolations

[Magellol]

(pre-)nested interpolations yet

Hmm. Just so I understand, if something is nested that means it got flattened right? But we're going to lint against english translations and we don't commit flattened format in english files I think 🤔

[samhh]

They can show up prior to flattening, for example:

{hasCollection, boolean, true {<collectionEl></collectionEl>} false {one of their collections}}

[Magellol]

@samhh Wasn't easy to figure out! Mind taking a look at cd03f4e? I didn't carry on with pattern matching the Plural case because it is actually quite complex and before I attempt to do it I was curious if my approach made sense? I think it's sound and my tests are passing but perhaps there is an easier method to this madness?

[samhh]

Pushed some updates. A lot of commits but @Magellol already did all the heavy lifting. 🙂 Summary:

• Finishes what @Magellol started - the pattern matching you didn't like is indeed something I'd like to improve that's come up before. Lenses might be one way. See: Traversal over streams is unergonomic #48
• Simplifies the core implementation via a getStreams abstraction. Fixes laziness in nested streams as well.
• Adds support for multiple lint rules so there's no friction for @nim442.

[samhh]

One more thing we need is to add CI output for the "internal" binary. That can wait for another PR.