Check for illegal characters when formatting names

[parsonsmatt]

This PR checks the type name for illegal characters before generating code.

[thomasjm]

Thanks! checkIllegalName seems not to be used anywhere though :P

Also, could you move the function to Util.hs or perhaps Formatting.hs?

FWIW, if something like this has a tendency to complicate the core code in TH.hs, it could also make sense to do it as part of the formatting step, e.g. the formatTSDeclaration function. At the formatting stage, the data types and logic are simpler and the TS names for things are more apparent. Might need an API change to allow formatTSDeclaration to error out in that case, although a pure exception doesn't seem out of the question--maybe a new function formatTSDeclarationEither. This all depends on whether it can be done cleanly in TH.hs of course.

[parsonsmatt]

Lol oops!

I put the function in the Util module as requested, and refactored it so it could be tested a bit more easily.

🤔

Right now, there's no option to customize the Haskell type name. So its definitely going to be a problem if we have any illegal characters there, and we can easily report the error as soon as we have the Name of the type we're deriving for.

I've modified the PR to also check datatype constructors.

---

And at this point I recognize that I've got a problem - the formatting options allow you to modify the interface and type names, which happens well after TemplateHaskell is done. That's a problem! We can't actually check at compile-time that the generated TS will be "correct" because we might change the typescript that actually gets written - getTypeScriptDeclarations for a given type isn't guaranteed to return the same typescript code that is actually printed by the app.

With this patch, anyone that is currently doing filter ('\'' ==) as a name modifier would get a compile-time error that doesn't really apply to them.

Oof.

I'd propose to modify the FormattingOptions and move those name formatters into the ExtraTypeScriptOptions. That gives you better consistency over the generated names, as well as allowing you to know at compile-time if the names are invalid.

[thomasjm]

Oh right, that's unfortunate. I don't love the idea of a breaking change to the name formatters API, although in hindsight it seems like the ExtraTypeScriptOptions might have been a better place for them. FWIW here's the PR that added the formatters: #5.

As an alternative hack, what about just replacing all invalid characters with underscores at formatting time? :)

Or just spitballing here, what about automatically converting invalid chars to an equivalent word, so ' becomes Prime etc. That seems like pretty good default behavior and would actually allow you to use your primed types in TS, rather than giving you a TH error.

[parsonsmatt]

As an alternative hack, what about just replacing all invalid characters with underscores at formatting time? :)

I think our linter complains about trailing _ 😅

A default name formatter that throws an error on an invalid character + tells you how to fix it may be the least invasive solution - we can change the formatter to be something like:

defaultNameFormatter str = do
  case NonEmpty.nonEmpty str of
    Nothing -> error "Name cannot be empty"
    Just nameChars -> case checkIllegalNameChars nameChars of
      Just badChars -> error $ concat ["The name ", str, " contains illegal characters: ", toList badChars, "\nConsider setting a default name formatter that replaces these characters." ]
      Nothing -> str

Obviously would want to trace the "how did i get here" so users can backtrack to exactly what failed + which name formatter needs to be invoked.

I do think the best choice is moving the formatting options to the TH side. Then you'd get a compile-time error, with a note to modify the name transformation field or the type itself. Given two steps, it wouldn't be a hard migration for end users:

1. Next major version: Release the new name transformation fields on ExtraTypeScriptOptions and deprecate the formatter fields w/ good migration messages
2. Major version after that: Delete the formatters.

[thomasjm]

Setting the default name formatter to error out as you describe sounds like a good first step. Maybe that would be sufficient to close #34 ?

If you get an informative error message at the formatting stage, would you still feel strongly about moving those options to the TH side?

[parsonsmatt]

Yeah, I generally want problems reported ASAP.

At least for our codebase, the derivation happens well before code generation, which is a totally separate step. I wouldn't want to introduce a runtime dependency on tsc for our test suite just to ensure that our generated code is well-formed.

[thomasjm]

I wouldn't want to introduce a runtime dependency on tsc

Huh? There's no tsc involved in the idea of erroring out at the formatting stage by using checkIllegalNameChars.

Just thinking through the implications of moving the formatting options -- it could harm at least one workflow I can imagine. What if you have lots of primed types like Foo' and Bar', and you're accustomed to bulk-formatting them by calling formatTSDeclarations' with an interfaceNameModifier that does T.replace "'" "Prime"? If we move the options, then such a user would have to individually apply formatting options to all their types at the deriveTypeScript call. (Assuming most people derive instances individually and format in bulk, like I do.) I suppose if we go through the deprecation process then people will be free to complain at that point.

[parsonsmatt]

Yeah, that would require a migration of code - but it shouldn't have to be a hard one. At work we consolidate our default options, so we can easily control what that default is by altering a single value in a single module. Factoring out the duplication in argument passing wouldn't be a huge problem.

[thomasjm]

All right, what do you think of the following?

1. In this PR, do the erroring defaultNameFormatter
2. Make a subsequent PR moving the formatting functions to the ExtraTypeScriptOptions, and maybe ask for comments from @Reisen

[parsonsmatt]

Sounds good! I'll make the relevant changes.

[parsonsmatt]

OK, should be ready for review :)

[thomasjm]

Looks great! Just a few comments.

[thomasjm]

Sorry, this fell off my radar -- merging now!

[thomasjm]

Automatic merge failed, manually rebased and merged in ba5d998