Skip to content

Conversation

@axelson
Copy link
Contributor

@axelson axelson commented Mar 8, 2025

Fixes a typo in the existing docs and switches to doctests to ensure that the docs do not get out of date.

But I am not quite 100% convinced that this is necessarily more readable, it feels a bit awkward to me. I wish I could write a better assertion as the last line of the doctest, we could use a heredoc but then we'd have to annoyingly deal with the trailing newline.

Here's the new and old docs:

Old New
Screenshot 2025-03-08 14-17-05@2x Screenshot 2025-03-08 14-17-27@2x

Contributor checklist

  • Bug fixes include regression tests
  • Features include unit/acceptance tests

Fixes a typo in the existing docs
@axelson axelson changed the title Switch to doctest for Igniter.Code.Common Switch to doctest for Igniter.Code.Common.add_code/3 Mar 8, 2025
@zachdaniel
Copy link
Contributor

Hmm...yeah that's interesting. The value of the secrets is pretty high even if it looks a bit less good. When you click the button to copy this do you get all the ...> also? Maybe we can clean it up by reworking the example to be single line strings in places?

@axelson
Copy link
Contributor Author

axelson commented Mar 8, 2025

When you click the button to copy this do you get all the ...> also? Maybe we can clean it up by reworking the example to be single line strings in places?

Clicking the button (or selecting the text directly) doesn't include the ...>. If you generate the docs with my makeup_elixir PR elixir-makeup/makeup_elixir#39 then you get:

existing_zipper = Igniter.Code.Common.parse_to_zipper!("""
IO.inspect("abc")
""")

existing_zipper
|> Igniter.Code.Common.add_code("""
IO.inspect("Goodbye, world!")
""", after: true)
|> Sourceror.Zipper.root()
|> Sourceror.to_string()
"IO.inspect(\"abc\")
IO.inspect(\"Goodbye, world!\")"

which can be directly copied and pasted to an IEx terminal that has Igniter as a dep.

But yeah it isn't really the most readable. Maybe it would make more sense to split it into two, a smaller more limited doctest and a larger by hand example (I think we could make that larger example flow a bit better than the current version on main).

@axelson
Copy link
Contributor Author

axelson commented Mar 8, 2025

The value of the secrets is pretty high even...

Do you mean doctest instead of secrets?

@zachdaniel
Copy link
Contributor

Yes, that was autocorrect 😂

@zachdaniel
Copy link
Contributor

I'm down with this. It is slightly less readable but more conventional.

@zachdaniel
Copy link
Contributor

Good to merge?

@axelson
Copy link
Contributor Author

axelson commented Mar 9, 2025

Yeah, let's do it! 🎉

@zachdaniel zachdaniel merged commit b9fa848 into ash-project:main Mar 9, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants