search

unsplash / intlc

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

Add emoji lint rule [UNS-1224] #128

Closed nim442 closed 2 years ago

nim442 commented 2 years ago

I am checking for emoji in the text part of Interpolation but I don't think I need to do it there

My formatter is not formatting with stylish-haskell for some reason. Will fix formatting once I figure out what's wrong

linear[bot] commented 2 years ago
UNS-1224 ICU messages: disallow emojis/Unicode chars

See [UNS-1056](https://linear.app/unsplash/issue/UNS-1056/before-zipping-convert-emojis-to-unicode) for context. This should be handled via linting, like [UNS-1168](https://linear.app/unsplash/issue/UNS-1168/icu-messages-disallow-nesting-function-inside-function).

samhh commented 2 years ago

If it's any help I think that test failure isn't a regression, it's just that it needs altering to call that lint rule directly.

We're testing that the interpolation rule stops iterating by placing an error value after the second interpolation (which works because Haskell is lazy). With your new rule added to lint that error value is being read, as you'd expect.

nim442 commented 2 years ago

@samhh Do you think creating our own show instances to format the lintingError ADT is a bad idea? I feel like it's a bit unidiomatic? Because if someone is playing around in repl, they will get shown the custom show instance and may not know what the actual ADT is?

samhh commented 2 years ago

@nim442 Yeah I think you're right. We could replace it with a plain a -> Text function.

samhh commented 2 years ago

Looking good! I guess the only thing to wait for now is clarification on exactly which characters to disallow more narrowly than isAscii.

On that note, I suspect we could create our own alternative function without the need for an external library: the implementation of isAscii looks straightforward.

OliverJAsh commented 2 years ago

Where did we get to with this?

nim442 commented 2 years ago

I think this is good to be merged. We were waiting at one point for the vendor to confirm if there were other things we needed to add but I think we are good on that front?

Probably needs this and the respective PR before we can start linting in the unsplash-web CI