klarna / erlavro

Avro support for Erlang/Elixir (http://avro.apache.org/)
Apache License 2.0
131 stars 39 forks source link

add support for map type #86

Closed ananthakumaran closed 5 years ago

ananthakumaran commented 5 years ago

I am not sure about the public API exposed by the library, so tried to keep all the exported function backward compatible.

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 556


Totals Coverage Status
Change from base Build 550: 0.0%
Covered Lines: 1697
Relevant Lines: 1697

💛 - Coveralls
k32 commented 5 years ago

Thanks for your PR! Could you please bump app version and add a changelog entry?

zmstone commented 5 years ago

unfortunately, we have a limit of maximum 80-char line width. please help to make the long lines shorter

ananthakumaran commented 5 years ago

I thought make elvis-rock would warn me about line length, I did fix all the errors reported by it. Btw, I didn't mean to trigger another review as I still need to address some of the review comments. I will ping you once I addressed all the comments.

zmstone commented 5 years ago

elvis does not check test modules.

zmstone commented 5 years ago

this diff should make elvis check test dir, please help to add it:


index c1391d2..7a420db 100644
--- a/elvis.config
+++ b/elvis.config
@@ -57,7 +57,15 @@
                    , {elvis_style, no_debug_call}

                  ]
-       }
+       },
+      #{dirs => ["test"],
+        filter => "*.erl",
+        rules => [ {elvis_style, line_length,
+                      #{ limit => 80,
+                         skip_comments => false
+                       }}
+                 ]
+      }
      ]
     }
    ]```
zmstone commented 5 years ago

@ananthakumaran Perfect PR!