Closed kinopyo closed 5 years ago
Merging #17 into master will increase coverage by
0.02%
. The diff coverage is100%
.
@@ Coverage Diff @@
## master #17 +/- ##
==========================================
+ Coverage 99.33% 99.36% +0.02%
==========================================
Files 5 5
Lines 150 157 +7
==========================================
+ Hits 149 156 +7
Misses 1 1
Impacted Files | Coverage Ξ | |
---|---|---|
lib/jsonapi/include_directive.rb | 100% <100%> (ΓΈ) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Ξ = absolute <relative> (impact)
,ΓΈ = not affected
,? = missing data
Powered by Codecov. Last update df8965f...046cb68. Read the comment docs.
@beauby may I get some feedback π π (before I forget what I was doing π )
Hi @kinopyo β will do it tomorrow hopefully (do not hesitate to ping me if I don't !)
@beauby (human low tech ping π π )
Hi @kinopyo β thanks for bearing with my slow reviews! ;)
While the current approach has the benefit of listing all invalid keys, I'd rather have the initializer raise InvalidKey
as soon as it hits the first invalid key. There are mainly two reasons why I prefer this fail fast strategy:
raise InvalidKey
if invalid), and a smaller overhead.What do you think?
@beauby oops sorry for keeping you waiting π , and many thanks for your inputs! I think the fail-fast strategy is ok, and the gain of less code is more rewarding π I've updated the PR and resolved Hound comments, what do you think?
@beauby Thank you for you quick review! Pushed 2 fixes π (and let me know if you prefer to have a single rebased commit before merging π )
Looks good, will merge tonight! Thanks for following up with this! :+1:
(Follow up https://github.com/jsonapi-rb/jsonapi-renderer/issues/16)
Raises
InvalidKey
exception when initialization:(@beauby Still quite a rough implementation, just to see if this is something we want. If so I'll continue do some refactoring, handling hounds and probably extracting the validation part to its own class π)