-
Notifications
You must be signed in to change notification settings - Fork 17
Support OTP 28 #91
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
Support OTP 28 #91
Conversation
generate_strict and b_generate_strictgenerate_strict, b_generate_strict, m_generate_strict
generate_strict, b_generate_strict, m_generate_strictgenerate_strict, b_generate_strict, m_generate_strict, zip
39133b3 to
48fdf42
Compare
elbrujohalcon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My goal with #89 was to make these changes but also compare OTP28's epp_dodger against OTP27's epp_dodger and adjust ktn_dodger accordingly… like we usually do with every OTP version update.
Then, based on that, adjust ktn_code, the tests, and everything else here.
I believe we can wait a little bit more and do the whole move to OTP28 properly instead of introducing just what we need for Elvis to handle OTP28 files.
Sure, or we can incrementally improve this via the open issue. I'm not sure how big the PR will end up being, but I must say I prefer smaller scope changes. The same reasoning applies to the Elvis Core change: better to do it incrementally. In any case, I'll separate the other one in two so it's easier to reason on. |
|
My perspective is that I prefer a single PR, with all the changes, to be sure we don't miss anything. |
I won't argue with this preference, as long as when the final PR comes you don't ask me to split it up 😆 |
generate_strict, b_generate_strict, m_generate_strict, zip|
@elbrujohalcon, shall we up the minimum to OTP 26 when this pull request is ready for review? |
|
I tentatively assigned this to 2.4.0. |
Pinky promise. |
YES! |
|
@elbrujohalcon, I think I was pretty thorough in the code comparisons (and we seem to cover Done: test code was parse_erl_for(Dir) ->
case file:list_dir(Dir) of
{ok, Entries} ->
lists:foreach(
fun(Entry) ->
FullPath = filename:join(Dir, Entry),
case filelib:is_dir(FullPath) of
true ->
parse_erl_for(FullPath);
false ->
filename:extension(Entry) =:= ".erl" andalso
try
{ok, Binary} = file:read_file(FullPath),
ktn_code:parse_tree(Binary)
catch
Class:Reason:S ->
erlang:display({exc, Class, Reason, FullPath, S})
end
end
end,
Entries
);
{error, _Reason} ->
error
end.3801 analysed, and the output is expected {error,function_clause,"<path_to_otp>/lib/xmerl/doc/examples/sdocbook2xhtml.erl"}
handled {throw,{unhandled_abstract_form,{error,{{6,2},ktn_dodger,{warning,"-warning ( \"a warning\" )."}}}},"<path_to_otp>/lib/tools/test/make_SUITE_data/test1.erl"}
expected {error,{badmatch,{error,{{38,12},erl_scan,{illegal,string}},{38,12}}},"<path_to_otp>/lib/wx/api_gen/wx_extra/wxTaskBarIcon.erl"}
expected {error,{badmatch,{error,{{72,12},erl_scan,{illegal,string}},{72,12}}},"<path_to_otp>/lib/wx/api_gen/wx_extra/wxPrintout.erl"}
expected {error,{badmatch,{error,{{32,12},erl_scan,{illegal,string}},{32,12}}},"<path_to_otp>/lib/wx/api_gen/wx_extra/wxListCtrl.erl"}
expected {error,function_clause,"<path_to_otp>/lib/edoc/test/edoc_SUITE_data/un1.erl"}
expected {error,function_clause,"<path_to_otp>/lib/edoc/test/edoc_SUITE_data/un3.erl"}
expected {error,function_clause,"<path_to_otp>/lib/ssh/examples/ssh_sample_cli.erl"}
handled {throw,{unhandled_abstract_form,{error,{{32,2},ktn_dodger,{warning,"-warning ( \"Ignore me -- testing that the debugger can handle warnings\" )."}}}},"<path_to_otp>/lib/common_test/test/ct_auto_compile_SUITE.erl"}
handled {throw,{unhandled_abstract_form,{error,{{34,2},ktn_dodger,{warning,"-warning ( \"Ignore me -- testing that the debugger can handle warnings\" )."}}}},"<path_to_otp>/lib/debugger/test/andor_SUITE.erl"}
expected {error,function_clause,"<path_to_otp>/lib/compiler/test/compile_SUITE_data/bad_enc.erl"}I'm gonna check if any of this is fixable, but these seem like corner cases, basically. Yeah, so this results in a further commit for a fix, but all the other cases are basically the UTF-8 -related oddities being tested. |
elbrujohalcon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it sucks and I'm truly sorry to request this but… I would like changes to ktn_dodger to be just the new additions (i.e., no formatting, no function renaming, no atom enclosing, etc…).
The docs as comments is clever. I didn't do it last time because it didn't occur to me that I could just put the new -doc attributes in comments. Those ones should stay :)
No a big issue; I'll spend some time moving this back, but the list of changes will probably be smaller, in the end. |
And keep the comment for historical reasons
272478a to
18d80c7
Compare
|
|
bb35ff4 to
44f7a51
Compare
|
Feel free to merge and release; I think I addressed all your comments. I'll then import the new version next to |

I add the minimum requirements to haveelvis_corenot throw (and parse) for upcoming rule changeoperator_spaces(around<:=and<:-, for lists, binaries and maps)We could think about movingminimum_otp_vsnto 26, now that 28 is out.I changed the scope of the PR (was what is now struck through; now is what the title states) following discussions around preferences. I'll move it to draft until I have time to work on more changes.
Fix #89.
The process
For future reference:
epp_dodgerover a formatted version ofktn_dodgerand adjusted the latter accordingly (take care to not ruin the "format" in place - eases review?).rebar3 testwas ✅absform.md(https://github.com/erlang/otp/blob/master/erts/doc/guides/absform.md) and the mentioned modules:epperl_evalerl_linterl_parseerl_ppabsform.mdℹ️ this was possibly the most useful of the buncherl_syntaxerl_prettyprInteresting commits: