unsplash / intlc

Compile ICU messages into code. Supports TypeScript and JSX. No runtime.
MIT License
56 stars 3 forks source link

Maintenance: linting #123

Closed Magellol closed 2 years ago

Magellol commented 2 years ago

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

Nice to have

Magellol commented 2 years ago

@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 commented 2 years ago

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.

Magellol commented 2 years ago

@samhh Yes! I think 0742643...f79160c does the trick

Magellol commented 2 years ago

(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 commented 2 years ago

They can show up prior to flattening, for example:

{hasCollection, boolean, true {<collectionEl></collectionEl>} false {one of their collections}}
Magellol commented 2 years ago

@samhh Wasn't easy to figure out! Mind taking a look at https://github.com/unsplash/intlc/commit/cd03f4e3e4620f4b5f14f41d4f9a41bde66e101c? 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 commented 2 years ago

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

samhh commented 2 years ago

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