Skip to content

Conversation

feliperenan
Copy link
Contributor

This commit moves the line and column to the last argument to be handled as a metadata as you can see in the example below:

    assert T.tokenize('foo', 1, 1, @opts) ==
             {:ok, [{:text, 'foo', %{line: 1, column: 1}}, {:eof, %{line: 1, column: 4}}]}

@josevalim
Copy link
Member

Looks great to me, we just need to get CI green! :)

@josevalim
Copy link
Member

Ah, don't forget to update the docs at the top of EEx.Tokenizer. :)

@feliperenan
Copy link
Contributor Author

feliperenan commented Feb 24, 2022

Yeah. I'm trying to debug what I'm missing here to make it green.

Also, we are going to expose token(contents, opts), right?

@josevalim
Copy link
Member

We will expose EEx.tokenize(contents, opts), yes!

This commit moves the line and column to the last argument to be handled
as a metadata as you can see in the example below:

```elixir
    assert T.tokenize('foo', 1, 1, @opts) ==
             {:ok, [{:text, 'foo', %{line: 1, column: 1}}, {:eof, %{line: 1, column: 4}}]}
```

This also expose Eex.tokenize/2 which takes line and column as options.
@feliperenan feliperenan force-pushed the frg-improve-eex-tokenizer branch from f38a617 to b032662 Compare February 24, 2022 23:50
@feliperenan
Copy link
Contributor Author

@josevalim I've updated the docs and exposed Eex.tokenize/2. I couldn't figure what is missing to get tests green though, I'm still investigating, but let me know if you have any clues - I guess it is something silly I haven't changed 🤔

@josevalim
Copy link
Member

josevalim commented Feb 25, 2022

@feliperenan can you also please change the tokenizer errors to be of the shape {:error, reason, meta}?

@feliperenan
Copy link
Contributor Author

@josevalim Done!. Let me know if there is something else 🙌🏻

@josevalim josevalim merged commit 1395240 into elixir-lang:main Feb 25, 2022
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@feliperenan feliperenan deleted the frg-improve-eex-tokenizer branch February 25, 2022 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants