Fix enums when using packages #49

Merged
merged 2 commits into from Jun 6, 2013

Conversation

Projects
None yet
6 participants
Contributor

d-mart commented Mar 29, 2013

Fix for #43

Use of a package name and enums was breaking compilation.
protobuffs_compile:atomize/1 is passed three variants of arguments
when using enums and packages:

  "myenum"
  [ "myenum" ]
  [ "myenum", "my.package_myenum" ]

Add additional function clauses for atomize to handle the variants.

@d-mart d-mart Fix enums when using packages
Use of a package name and enums sends was breaking compilation.
protobuffs_compile:atomize/1 is passed three variants of arguments
when using enums and packages:
  "enum_name"
  [ "myenum" ]
  [ "myenum", "my.package_myenum" ]
51e0d7b

Is this one not just a special case of the next one where _Rest is empty? It seems like we're only using the first thing in the list anyway right?

tra commented Mar 29, 2013

@jsanders, the 2nd case is not quite a special case of the first one.

[X|[Y]] = ["a"].
** exception error: no match of right hand side value ["a"]

And lest you think you can do this...

[X|Y] = ["a", "b"].
** exception error: no match of right hand side value ["a","b"]

So it seems you need both variants to handle all the cases.

tra commented Mar 29, 2013

@jsanders on second thought, you may be right. I had a bug in the above runs where I had already assigned Y to something. So this case might very well work for both of the first 2 cases...

atomize([String|_Rest]) when is_list(String) ->
    atomize(String);

tra commented Mar 29, 2013

@jdsanders I'm wrong again... it needs to be split up because of the way erlang handles strings. Otherwise this gives unexpected results...

> [X|Y] = "abc".
"abc"
> X.
97
> Y.
"bc"

So the original pull request is perfect. 😄

@tra Glad I could be of so much help 😄

@seancribbs seancribbs commented on an outdated diff Apr 21, 2013

src/protobuffs_compile.erl
atomize(String) ->
+ io:format("Atomizing ~p~n", [String]),
@seancribbs

seancribbs Apr 21, 2013

Contributor

Please remove this line.

nygge commented Apr 24, 2013

I believe the correct fix for this is to add

Enums = canonize_names(RawEnums),

in output_source/4 line at 160, in the same way as in output/4.
You can verify it by checking that it is only generate_source, and not scan_file, that fails.

Contributor

djnym commented Jun 6, 2013

Any sense when this will be merged, we've run into this problem, and I'd prefer not to maintain a long lived fork with this patch.

Contributor

seancribbs commented Jun 6, 2013

@djnym I'll have to merge it manually, the submitter did not remove the stray io:format line.

Contributor

d-mart commented Jun 6, 2013

@seancribbs I meant to when I pushed up ce5c5e0 but it seems it's still there. I will remove it and leave a comment here when it's up later today.

Contributor

seancribbs commented Jun 6, 2013

@d-mart Thank you.

@d-mart d-mart Add guards to atomize
The first two function clauses were matching one- and two-character
symbol names.  Add guards to ensure the argument is a list of strings,
not just a single string (list).
0ead943
Contributor

d-mart commented Jun 6, 2013

Ok, I think it's good-to-go (finally)! 👍

Contributor

seancribbs commented Jun 6, 2013

Before I merge this, is there a test that would incur this problem?

Contributor

d-mart commented Jun 6, 2013

In general, it was meant to fix #43 - the example proto file in the comments would cause a crash. I'll try to get a proper test added to this PR.

Contributor

seancribbs commented Jun 6, 2013

@d-mart Thanks, reviewing both.

Contributor

seancribbs commented Jun 6, 2013

AFAICT this is ok, merging.

@seancribbs seancribbs added a commit that referenced this pull request Jun 6, 2013

@seancribbs seancribbs Merge pull request #49 from d-mart/issue_43
Fix enums when using packages
77848da

@seancribbs seancribbs merged commit 77848da into basho:master Jun 6, 2013

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment