-
Notifications
You must be signed in to change notification settings - Fork 152
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
Snake case support, by_proto prefix, oneof/maps fix #76
Snake case support, by_proto prefix, oneof/maps fix #76
Conversation
Thanks, will take a look |
" > include/gpb_version.hrl "}, | ||
{compile, | ||
%% way of invoking shell script: see above | ||
"sh build/compile_descriptor"} |
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 don't think moving the compile_descriptor from a post-hook to a pre-hook is good.
It is a two-stage process to build gpb (or three-stage if you include building the gpb_version.hrl
):
- First build the compiler
- Then use the compiler to build an encoder for
.proto
files. This is the descriptor. It can be used to add a protobuf-encoded description of the proto file(s) into generated.erl
files, if thedescriptor' (
-descr') option is specified.
On this branch, make clean
followed by rebar compile
now leads to:
% make clean
rm -f include/gpb_version.hrl ...
...
% rebar compile
==> gpb (compile)
Compiling descriptor.proto...
{"init terminating in do_boot",{undef,[{gpb_compile,c,[['descriptor.proto']],[]},{init,start_it,1,[]},{init,start_em,1,[]}]}}
Maybe there's some other way to make this work with both rebar2 and rebar3? (not knowing what the problem with rebar3 was)
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.
Aah. Well damn.. hm. So with rebar3 it does not compile to ./ebin
but to _build/default/lib/gpb/ebin
. With the use of pre
the hook would write to ./ebin/
which gets copied to _build/default/lib/gpb/ebin
when things are compiled.
I guess I'll have to add a check to have it compile to _build
if it exists and ./ebin
if it exists.
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.
Ah, I see, didn't think of it compiling to _build/
even though I've seen it
236a78d
to
08d96ff
Compare
input=msg_name_1, %% both argument ... | ||
output=msg_name_2} %% .. and result msgs to be to-lower | ||
]}] = do_process_sort_defs(Defs, [msg_name_to_snake_case]). | ||
|
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.
Maybe it could be worth adding also unit tests for the oneof/map crash in #75, for example the below (this is what I used locally to verify the change)
can_tolower_record_names_with_oneof_test() ->
{ok, Defs} = parse_lines(["message Msg1 {",
" oneof u {",
" Msg1 a = 1;",
" uint32 b = 2;",
" }",
"}"]),
[{{msg,msg1}, [#gpb_oneof{fields=[#?gpb_field{name=a,type={msg,msg1}},
#?gpb_field{name=b}]}]}] =
do_process_sort_defs(Defs, [msg_name_to_lower]).
can_tolower_record_names_with_map_test() ->
{ok, Defs} = parse_lines(["message Msg1 {",
" required uint32 f = 1;",
"}"
"message Msg2 {"
" map<string,Msg1> m = 1;",
"}"]),
[{{msg,msg1}, [#?gpb_field{}]},
{{msg,msg2}, [#?gpb_field{type={map,string,{msg,msg1}}}]}] =
do_process_sort_defs(Defs, [msg_name_to_lower]).
Name1 = maybe_tolower_or_snake_name(Name, ToLowerOrSnake), | ||
Prefix1 = case Prefix of | ||
{by_proto, PrefixList} -> | ||
proplists:get_value(Name1, PrefixList, ""); |
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.
hmm... just remembered I hadn't taken a look at this part. Sorry for doing it late. And also sorry in advance for bringing up a naming issue. :)
The prefix naming adds the option to specify prefix for individual messages on a per-message basis. Earlier, we discussed prefixing on a per-proto-file basis. I'm fine with either way, I think, but now that it is on a per-message basis, I see two things:
- The
by_proto
name should be something else.by_msg
comes to mind, but I'm of course open to other alternatives - The key that one should specify for prefixing messages is the name after any conversions (to-lower/snake etc) but since this is far from obvious, I think the
by_msg
(or whatever) sub-option would deserve a line or two in the (oh-so-long) doc text forgpb_compile:file
.
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.
Hah, I keep thinking "messages" are "protos" and not "messages in a proto". I'll look at making it actually by_proto
. Probably have that tomorrow.
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.
Hm, post_process_all_files
has all msgs, including imported, already in the list of defs and no way (as far as I can tell) to know which proto each message came from, so I don't think that is going to work.
Should add that I've run my usual battery of tests on a local test-merge (of just-refetched b6e67e5), and from that point of view, things look good. |
I updated it to be |
Yes, one would have to keep track of which messages belong to which files, roughly like this:
|
Ok, this sounds workable, taking a crack at it now. |
Updated back to |
Imports = gpb_parse:fetch_imports(Defs), | ||
Opts2 = ensure_include_path_to_wellknown_types_if_proto3( | ||
Defs, Imports, Opts), | ||
MsgContainment = {{msg_containment, ProtoName}, [Name || {{msg, Name}, _} <- Defs]}, |
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.
Line is overlong
Looks generally fine, but one eunit test is failing, the fix should be trivial, I should think. The rest of the tests pass. in function gpb_compile_tests:parses_file_to_msg_defs_test/0 (test/gpb_compile_tests.erl, line 180)
**error:{badmatch,{ok,[{{msg,'Msg'},[{field,field1,1,2,uint32,required,[]}]},
{{msg_containment,"X"},['Msg']}]}}
output:<<"">> After this, I'd like to suggest squashing some commits, and then I'd be ready to merge it. |
Updated and squashed to 2 commits. |
Thanks! Merged and included in just-pushed 3.25.0 (closing) |
This PR covers the issues: #73 #74 and #75
In addition to adding
msg_name_to_snake_case
,by_proto
and the fixes for oneof/maps this also has a few changes to make it work better with rebar3. Mainly moving thecompile_descriptor
hook to apre_hook
instead ofpost
.