Skip to content

Handle array for with option #8

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.

Sign up for GitHub

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
Merged
merged 3 commits into from
May 30, 2019
Merged

Handle array for with option #8

merged 3 commits into from
May 30, 2019

Conversation

alex-lairan
Copy link
Contributor

The with option should handle Arrays.

Fix : #7

@alex-lairan alex-lairan force-pushed the feature/support_array_with_option branch from 98976a7 to 6e13771 Compare May 26, 2019 16:38
@alex-lairan alex-lairan force-pushed the feature/support_array_with_option branch from 6e13771 to ea695c2 Compare May 26, 2019 16:49
c910335
c910335 requested changes May 29, 2019
View reviewed changes
Copy link
Owner

@c910335 c910335 left a comment

Choose a reason for hiding this comment

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

Sorry for late response.

The documentation of with option (src/crinder.cr L86) should also be fixed.

spec/crinder_spec.cr
@@ -47,6 +47,18 @@ class NilableTodoRenderer < TodoRenderer
remove updated
end

record TodoSubtasked, name : String, subtasks : Array(Subtask)
record Subtask, name : String
Copy link
Owner

@c910335 c910335 May 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I prefer moving these two records to L4, under record Todo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think fixtures should be simple and focused to what's important.

@alex-lairan alex-lairan force-pushed the feature/support_array_with_option branch from ea695c2 to 52236d0 Compare May 29, 2019 16:49
@alex-lairan
Copy link
Contributor Author

alex-lairan commented May 29, 2019 •
edited
Loading

Hi !

Thanks for the review.

About this documentation :

# - **with**: a renderer for this field. This field will be filtered by `value` before passing to it. It is not necessary to be a subclass of `Crinder::Base`, but it must have the class method `render(object : T, json : JSON::Builder)` where `T` is the original type of this field.

I need to change render(object : T, json : JSON::Builder) to render(object : T | Array(T), json : JSON::Builder) ?

@c910335
Copy link
Owner

c910335 commented May 29, 2019

@alex-lairan

I need to change render(object : T, json : JSON::Builder) to render(object : T | Array(T), json : JSON::Builder) ?

Yes

@alex-lairan
Copy link
Contributor Author

@c910335 I've finished the documentation :)

@c910335 c910335 merged commit a0d487a into c910335:master May 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c910335 c910335 c910335 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.

Using "with" Option to Render an Array
2 participants
@alex-lairan @c910335