Normalize types, typespecs and module attributes #52
Conversation
jiegillet
added
x:module/representer
There was a problem hiding this comment.
I didn't analyze every single added line, but the new tests are very extensive and should have detected potential issues, so LGTM 🙁
I made a bunch of other small changes and refactors, including making almost all functions private. I know it's not good practice for an established codebase, but this one is not fully mature yet.
Good choice - better sooner than later!
lib/representer.ex
Outdated
| # replacing function names and variables | ||
| |> Macro.prewalk(mapping, &use_existing_placeholders/2) | ||
| # dropping docs | ||
| |> Macro.prewalk(&drop_docstring/1) | ||
| # removing added metadata | ||
| |> Macro.prewalk(&drop_line_meta/1) | ||
| # adding parentheses in pipes | ||
| |> Macro.prewalk(&add_parentheses_in_pipes/1) |
There was a problem hiding this comment.
I'm not sure those comments are useful. They're almost the same as the function names.
There was a problem hiding this comment.
You're right, they were very helpful to me as I was restructuring the code, but they don't need to stay.
| # hack: assuming that if a module has no complete placeholder name, that means it's not being defined in this file | ||
| # TODO: fix when dealing with aliases | ||
| nil |
There was a problem hiding this comment.
Does removing the TODO comment mean we're giving up on fixing this, or is it no longer considered a hack but the final solution?
There was a problem hiding this comment.
Actually I interpreted this line as "this doesn't work with aliases yet". I added alias support in my previous PR but forgot to remove the comment. So my intent was not to comment on the hack.
I don't really have an issue with the "hack" or any plan to remove it or finalize it... It works, so I didn't touch it :)
| when placeholder_8: atom, placeholder_9: integer | ||
| @spec placeholder_10(placeholder_11) :: [placeholder_11] when placeholder_11: var | ||
| @callback placeholder_12(placeholder_19 :: placeholder_3(placeholder_1), placeholder_20 :: any) :: placeholder_10(any) | ||
| @macrocallback placeholder_13(placeholder_21 :: String.t()) :: Macro.t() |
There was a problem hiding this comment.
The PR description says:
I chose to represent all types without parentheses, as it's easy to read, the documentation uses that a lot and it made the implementation easier.
yet here's String.t() and Macro.t(). IMO it doesn't matter, but I just wanted to point out in case it's a bug in your opinion.
There was a problem hiding this comment.
Whoops, I missed that. Yeah, I would consider that a bug (or at the very least, it would bug me)
There was a problem hiding this comment.
Well... Looks like I'm going to have to live with it. I do have code that sets no_parens: true in the metadata at the right place, but it looks like this is something the formatter feels strongly enough to overwrite (in Macro.to_string since 1.13), and I have not found a way to change it.
Co-authored-by: Angelika Tyborska <[email protected]>
|
Thank you for the review!! I know it was a big one :) |
Closes #24.
Sorry for another large PR, this problem was trickier than I thought.
Basically, we have to run the representation twice, one for the types and typespecs, one for the rest. This is so we can keep the
integertypes in@spec function(integer :: integer) :: integerbut still replace the first one.This is also why I had to modify the
two_modulestests, since the traversal order changed.I chose to represent all types without parentheses, as it's easy to read, the documentation uses that a lot and it made the implementation easier.
I made a bunch of other small changes and refactors, including making almost all functions private. I know it's not good practice for an established codebase, but this one is not fully mature yet.