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

Setters for reference_time and timezone in Context. #22

Merged

Conversation

hfinck
Copy link
Contributor

@hfinck hfinck commented Aug 12, 2016

People should be able to set the reference_time and timezone to their liking so I added the appropriate setters for that.

As we only accept a date time interface we can make sure that it is properly formatted as required by wit.ai.

public function setReferenceTime(\DateTimeInterface $dateTime)
{
    $dt = $dateTime->format(DATE_ISO8601);
    $this->add('reference_time', $dt);
}

Same thing for the time zone. It will error out if a non-valid timezone string is applied.

We also make sure that an empty context is serialised to an empty JSON object. Otherwise we would get an empty array, which will be rejected by wit.ai with an error.

    public function jsonSerialize()
    {
        return $this->data ?: new \stdClass;
    }

*/
public function jsonSerialize()
{
return $this->data;
return $this->data ?: new \stdClass;
Copy link
Owner

Choose a reason for hiding this comment

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

I not a big fan of this. Why not just check if the context is not empty before send it like in my PR here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, the thing I wanted to make sure is that the Context does not get serialized into an array, since that would be serialized the wrong way.
In your PR you have a check in place where we do not send an empty Context. So for that matter it would be fine to just return the data array with no check at all. Alright with that? I then would restore that line to the previous state.

Copy link
Owner

Choose a reason for hiding this comment

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

@hfinck yes you right it can be usefull, but it's should not return a stdClass, return null will be better ? What do you think ?

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'm just returning data now. It would't be good to just return null. I can live with the empty array and taking care of it in the ConversationApi.

@hfinck hfinck force-pushed the remove_reference_date branch 2 times, most recently from 2c0513a to 809f46b Compare August 15, 2016 07:52
@tgallice
Copy link
Owner

👍
@hfinck Just fix the tests and I will merge it. Thank for this !

Also the serialization methods returns stdClass to enforce a json object in case the context is empty.
@hfinck
Copy link
Contributor Author

hfinck commented Aug 15, 2016

@tgallice All good. Go ahead.

@tgallice tgallice merged commit 928e04c into tgallice:remove_reference_date Aug 15, 2016
tgallice added a commit that referenced this pull request Aug 15, 2016
* Remove reference_date

* Do not send context if it is empty

* Setters for reference_time and timezone in Context. (#22)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants