Proposal: Deprecate {:ok, {:code, Macro.t()}} in favor of alternative form

[zachallaun]

Problem

Igniter introduces some idioms that span multiple utilities, the goal of which is making it easier/more natural to update code (usually zippers). One such idiom is callbacks returning :ok tuples that can take a number of forms:

{:ok, Zipper.t()} # replace the document zipper with the returned zipper
{:ok, {:code, Macro.t()}} # replace the zipper node with this quoted form
{:ok, {:code, String.t()}} # parse string to quoted, then replace zipper node

The problem is that the last two forms overlap: "my text!" is a valid quoted form, but Igniter instead parses it, raising if it's an invalid AST, or giving an unexpected result (like my(text!) in this example). This can be surprising, confusing, and difficult to debug if you haven't seen it before.

The solution isn't necessarily intuitive, especially if you're not experienced with Elixir AST: You have to return {:ok, {:code, {:__block__, [], ["my text!"]}}} or {:ok, Zipper.replace(zipper, "my text!")}.

Proposal

I think we should bite the bullet and deprecate the {:ok, {:code, Macro.t()}} form in favor of:

{:ok, Zipper.t()}
{:ok, {:code, String.t()}}
{:ok, {:quoted, Macro.t()}}

I'm not 100% sure how this should be done yet. There are a potentially large number of places where these forms are handled, and adding deprecation notices to each one may be tricky. We also may want to combine this proposal with some changes to Igniter.Code.Common.add_code/3 to distinguish between a string of code and a quoted form as well.

[zachdaniel]

Completely agree. This was one of the biggest whiffs of my part on the initial design 😆

I think what we should do is make special data structure, and require that we get that structure, and deprecate with a message whenever we get anything else (the zipper case can remain).

So I'd say something like add_code(zipper, Igniter.Code.parse_string("my_string"))

We could have one for values that should be escaped, one for quoted values that have to be stringified and then parsed w/ sourceror etc. (user doesn't need to know that).

It will be a struct with just one field to start ast which contains AST safe for insertion.

Thoughts?

[zachallaun]

Okay, I think I like that (especially if it can live in Igniter.Code, which is as-yet unused). Thinking through a potential API:

%Igniter.Code{ast: quoted}
Igniter.Code.from_string!("string of code")
Igniter.Code.quoted(quoted_form)
Igniter.Code.escaped(value) # for values like maps that need Macro.escape(value)

I personally prefer from_string! over parse_string because it's returning an %Igniter.Code{} and doesn't confuse it with Sourceror.parse_string. Also think it should use a bang since it will raise on invalid code.

[zachdaniel]

Perfect 👍

[zachallaun]

Cool. I probably won't be able to work on this until next week, but will plan to get it done if you don't get to it first. :)

[zachdaniel]

I probably won't have time in the next few weeks, but I think this change is one of the major blockers for a 1.0 release so it would be amazing to have it done. Not that we have to rush that milestone.

[zachallaun]

Was able to start on this here: https://github.com/ash-project/igniter/compare/za-code-struct?expand=1 (currently just defined the struct, not used anywhere yet)