Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Owl.Data.from_ansidata/1 #11

Merged
merged 6 commits into from
Mar 24, 2023
Merged

Conversation

zachallaun
Copy link
Contributor

@zachallaun zachallaun commented Mar 22, 2023

Implements #9. This isn't quite ready to merge, but wanted to open it as a draft for now. The primary things left are to add docs and quite a lot more tests.

Notes:

  • I've created an internal Owl.Data.Sequence module that takes a similar approach to IO.ANSI. It's responsible for identifying escape sequences, grouping sequences into types, knowing what the defaults are, etc.
  • maybe_wrap_to_tag, reverse_and_tag, and collapse_sequences are unchanged, just moved.
  • I'm not making an effort to "combine" tags in any intelligent way at the moment. See the nested red/underlined test for example. Other than for aesthetic purposes, I don't know that there's a real benefit to this and I believe it would make the implementation rather more complicated.

Example:

%{foo: 1, bar: "two", baz: 'three'}
|> Inspect.Algebra.to_doc(Inspect.Opts.new(syntax_colors: IO.ANSI.syntax_colors()))
|> Inspect.Algebra.format(80)
|> Owl.Data.from_ansidata()
|> Owl.IO.puts()

image

lib/owl/data/sequence.ex Outdated Show resolved Hide resolved
@zachallaun zachallaun marked this pull request as ready for review March 23, 2023 13:56
@zachallaun
Copy link
Contributor Author

Rebased with main, added additional tests, and updated docs!

lib/owl/data/sequence.ex Outdated Show resolved Hide resolved
@fuelen
Copy link
Owner

fuelen commented Mar 23, 2023

Played a bit with it. Looks nice, good job.
Just noticed that it doesn't work with binaries, so getting data from cmd and trying to parse it won't work. Technically, binary is a part of iodata

> "test" |> Owl.Data.tag(:red) |> Owl.Data.to_ansidata() |> to_string() |> Owl.Data.from_ansidata()
"\e[31mtest\e[39m\e[0m"

@zachallaun
Copy link
Contributor Author

zachallaun commented Mar 23, 2023

Just noticed that it doesn't work with binaries, so getting data from cmd and trying to parse it won't work. Technically, binary is a part of iodata

This is a documented limitation for now, see here.

It wouldn't be too hard to parse binaries to split out escape sequences in the future, but I chose not to add that for this first iteration. I agree that it would be nice to have in order to consume data from cmd. My personal use-case was focusing on consuming ansidata from IO.ANSI.

Edit to add: Another edge-case if we get into parsing escape sequences that are embedded in binaries: iodata doesn't care whether the sequence is split.

image

Given this, probably the best way to handle this would be to toss the existing code that traverses ansidata and just to_string(ansidata) and parse character-by-character when a \e is encountered.

@fuelen
Copy link
Owner

fuelen commented Mar 24, 2023

Yes, let's do binary parsing in a new iteration.

Thank you for the PR! ❤️

@fuelen fuelen merged commit 1b42999 into fuelen:main Mar 24, 2023
end

defp reverse_and_tag(sequences, [%Owl.Tag{sequences: last_sequences} | _] = data) do
maybe_wrap_to_tag(sequences -- last_sequences, Enum.reverse(data))
defp do_from_ansidata(integer, open_tags) when is_integer(integer) do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at coverage and this line is red. This is for char list, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it’s just to handle individual character literals in ansidata. [“hello”, \?]

Should’ve added a test, sorry!

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