Skip to content

Conversation

@mablae
Copy link

@mablae mablae commented Jul 17, 2016

This PR adds a result object that wraps the result object.

The result object has two helper methods: ->asArray() and ->asJson()
and delegates the property access via magic call to the wrapped result.

@osavchenko
Copy link
Owner

You are trying to make a lot of changes in one go. It's not ok.
Why do you need this response wrapper?

@mablae
Copy link
Author

mablae commented Jul 18, 2016

Hello @osavchenko,

Please see this just as a start of a WIP PR.

It is not ready yet! Thanks for your feedback!
I will try to to commit more eary more often. Sorry.

I could rebase it, and crunch it into smaller commits. On the other side rebase -i has to be done anyway, when this is ready. Just let me know, how I should split the big commit.
Is three commits (Tests / New Class / Use new class) okay?

The main intend of the new result Class was that stdClass is bad to typehint against.
I usually have named constructor pattern on my DTOs where I want to use it.
An interface would be even better.

I am still thinking about the response object. ATM I have no real personal usecase for wrapping it into the result object. It was just inspired from elasticsearch client lib "elastica": It returns an ResultSet with
the original response attached to it.

In the end it would be nice to have a "real" object with some methods like ::getTemperature($unit = Temperature::Celsius) with some internal logic to "enhance" the Developer XP and ease of use.

I dont know, which direction (if any) are you going with this Bundle?

Thank you for your feedback! 👍

environment:
php:
version: 7.0
version: 7.0.8
Copy link
Owner

Choose a reason for hiding this comment

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

Please, revert this change

Copy link
Author

Choose a reason for hiding this comment

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

Sure. It was just a blind guess. Scruntinizer still does not work.

@osavchenko
Copy link
Owner

I suggest to split this PR to 2 ones:

  1. Move test data to trait.
  2. Make data class.

Also, I think about caching responses (because openweathermap has own limits for requests) and store cached result into database with ORM or ODM.

About helper methods: ->asArray() and ->asJson(). We can use symfony serializer component or JMSSerializerBundle. With such serialization it will be more flexible. What do you think?

@mablae
Copy link
Author

mablae commented Jul 19, 2016

I suggest to split this PR to 2 ones:

I will do so.

Also, I think about caching responses (because openweathermap has own limits for requests) and store cached result into database with ORM or ODM.

Shouldn't caching be some of the regular cache? Like memcached, redis, filesystem.. ?
Using an ORM for caching is not what it was made for in first place.

I like the idea of (optional) caching!

About helper methods: ->asArray() and ->asJson(). We can use symfony serializer component or JMSSerializerBundle. With such serialization it will be more flexible. What do you think?

ACK - Symfony Serializer will get nice Annotation support like JMS already has in v3.2 btw.

@osavchenko
Copy link
Owner

Shouldn't caching be some of the regular cache?

What do you mean?

ACK - Symfony Serializer will get nice Annotation support like JMS already has in v3.2 btw.

I also like JMSSerializer :)

@mablae
Copy link
Author

mablae commented Jul 19, 2016

Shouldn't caching be some of the regular cache?

What do you mean?

I meant that an ORM is not an cache.
There are several better ways to "cache" sth , like redis, memcached, filesystem...

Maybe the lib could implement the PSR-6 Caching Standard and let the user decide what to use?

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.

2 participants