Skip to content
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

Jump to bottom

Unify string handling #29

Merged
merged 22 commits into from Mar 5, 2017
Merged

Unify string handling #29

merged 22 commits into from Mar 5, 2017

Conversation

zmstone
Copy link
Contributor

@zmstone zmstone commented Feb 17, 2017

No description provided.

@zmstone zmstone force-pushed the unify-string-handling branch 2 times, most recently from 6299879 to ae5d631 Compare February 26, 2017 09:27
@zmstone
Copy link
Contributor Author

zmstone commented Mar 4, 2017

@id not planing to add more for this PR.
Will work on code-generation in another PR.

id
id reviewed Mar 4, 2017
View reviewed changes
README.md Outdated
@@ -217,6 +225,10 @@ For a big union like below
]
```

### Caution when unioning string type and int/long arrays.

As `[integer()]` list is `string()` in Erlang, this will confuse the encoder, please make sure to `binary()` as avro string encoding input.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"make sure to use"

include/avro_internal.hrl Outdated
@@ -1,3 +1,4 @@
%%%-----------------------------------------------------------------------------
%%% Copyright (c) 2013-2016 Klarna AB
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update copyright

include/erlavro.hrl Outdated
@@ -1,3 +1,4 @@
%%%-----------------------------------------------------------------------------
%%% Copyright (c) 2013-2016 Klarna AB
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update copyright

scripts/cover-print-not-covered-lines.escript Outdated
%%! -smp enable -sname notcoveredlinessummary -pa ebin -pa ../ebin

%%%
%%% Copyright (c) 2015-2016, Klarna AB
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update copyright

scripts/cover-print-not-covered-lines.escript Outdated

%%%=============================================================================
%%% @doc
%%% @copyright 2016 Klarna AB
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update copyright

src/avro_schema_store.erl Outdated
@@ -124,162 +109,92 @@ import_files(Files, Store) when ?IS_STORE(Store) ->
lists:foldl(fun(File, S) -> import_file(File, S) end, Store, Files).

%% @doc Import avro JSON file into schema store.
%% In case the schema is unnaed, the file basename is used as its
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnamed

src/avro_union.erl Outdated
do_encode(Type, Union, EncodeFun) ->
MemberTypes = avro_union:get_types(Type),
try_encode_union_loop(Type, MemberTypes, Union, 0, EncodeFun).
%% @private Build the member type name to member idnex mapping.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index

@zmstone zmstone force-pushed the unify-string-handling branch 3 times, most recently from 2c4c369 to b78ce33 Compare March 5, 2017 13:03
@zmstone
Unify strings
390803a 
Type names, namespaces, enum symbols, map keys, record field names
are allowed to be atom() | string() | binary()
Atom, string, binary are allowed as 'string' type value
All strings are canonicalized as binary() internally
@zmstone
Merge readme tests to avro_tests.erl
b8c824a
@zmstone
Full coverage for avro_util.erl
cfe63c7
@zmstone
Full coverage for avro_union
69bfe5e
@zmstone
Allow assigned names for unnamed types in schema store
e75c44e
@zmstone
Remove cast from avro_value() supports
57c0be4
@zmstone
Full test coverage for avro_record
395b030
@zmstone
Remove 0 -> false 1 -> true
64eb133
@zmstone
Full coverage for avro_ocf
5771e17
@zmstone
Full coverage for avro_map
1e17ef0
@zmstone
Full coverage for avro_json_encoder
7cc4c4a
@zmstone
Full coverage for avro_json_decoder
96c12f7
@zmstone
Full coverage for avro_fixed
0e05af5
@zmstone
Full coverage for avro_binary_decoder
1ba9e44
@zmstone
Simplify enclosing namespace logic.
450c573 
Disadvantages of old logic:
1. 'enclosing_ns' option has to be given for named type defenitions
in order to have their full names set properly.
2. EnclosingNs variable has to be passed down the type decoding
call stacks in order to resolve children type full names.

After this change, children type full names are recursively
resolved when they are used to define a parent (named) type.
@zmstone
Update copyright
0260beb
@zmstone
Add primitive type shortcuts when defining types.
513ce1b 
When defining complex types, array / union / record
it is quite anoying if we have to call avro_pmititive:xyz_type()
to define primitive type.
In this commit, atom/binary/string for primitive type names are
allowed to use as shortcuts
@zmstone
Full coverage for avro.erl
e8a466e
@zmstone
Improve union internals
33f2af2
@zmstone
Make dialyzer happy
b6c1c63
@zmstone
Update README.md
c146fbc
id
id approved these changes Mar 5, 2017
View reviewed changes
Copy link
Contributor

@id id left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! 💯 👍

@zmstone zmstone merged commit 90963ce into 2.0-dev Mar 5, 2017
@zmstone zmstone deleted the unify-string-handling branch March 5, 2017 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@id id id approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@zmstone @id