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

Solicit List Support #44

Closed
wants to merge 2 commits into from
Closed

Solicit List Support #44

wants to merge 2 commits into from

Conversation

JosephShering
Copy link
Collaborator

as_json now has an extra pattern and guard matching lists.

as_json now supports an elixir list.
@JosephShering JosephShering added enhancement New feature or request help wanted Extra attention is needed proof of concept labels Aug 31, 2021
@@ -471,6 +471,14 @@ defmodule Solicit.Response do

def as_json(nil, _fields), do: nil

def as_json(list, fields) when is_list(list) do
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion:
having a key - value so the response would look something like

{
   "records": [
         %{
           "step" => 0,
           "name" => "test"
         },
         %{
           "step" => 1,
           "name" => "test 2"
         },
         %{
           "step" => 2,
           "name" => "test 3"
         }
   ]
}

https://smartrent.atlassian.net/wiki/spaces/ENG/pages/855408690/API+Standards#JSON-Shape

Copy link
Member

Choose a reason for hiding this comment

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

@tmadar Definitely agree that we should be sticking with with our standards as much as possible, but wouldn't that make it impossible to have as_json return a list as the root element?

Copy link
Member

Choose a reason for hiding this comment

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

Ya, we are migrating some existing endpoints that do not follow our API standards and solicit blocks us here because it does not support lists as a root element. After meeting about it today, I think we will actually end up creating v2 endpoints that follow the standard rather than patching solicit to support non-standard functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed proof of concept
Development

Successfully merging this pull request may close these issues.

4 participants