Open xberger opened 2 years ago
Hey,
Thank you very much for your time !
Tweet interface: Obviously, API v1 and v2 are parting ways now even more. How to deal with that in the Interface
Tweet
and in the classTweetV1
with the v2 only getters likegetImpressionCount()
? Now it returns 0. Should it better throw an exception? Return -1? Changing return type to return null?
Good question, personally I would throw an exception (and use @JsonIgnore
to avoid calling the methods during serialization) but as you are currently the only one using it, you can decide how you want to implement it. Maybe later other users will challenge this choice.
For the other points, it looks relevant.
Finally for the tests, you should create json sample files and try to deserialize them and be sure that all fields are OK (as I did for TweetV2 object for example).
Hi,
thanks for the feedback. Will work on it. At the moment I'm still trying requests and learn something about the partial API errors.
Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Our GitHub checks need improvements? Share your feedbacks!
Did you leave it up ? Can I close it?
Did you leave it up ? Can I close it?
Well.. I ran out of breath for this pull request back in March and just left it as it was in my project. In retrospect it should not have been a pull request at this point, since the error handling I added was erroneous.
Now I want to improve the error handling, so I merged. I think there must be dto objects for mixed state (where you have returned objects and error messages at the same time). Like the class TweetError, but that class is never used, as far as I can see?
So I'm now working on it again, so you can leave it open at the moment.
Well, now non-public data and error handling are intertwined, which is not good. I should maybe revert the error handling to have a clean state for this pull-request. Sorry for the chaos.
@redouane59 I did another update with some minor changes discusses earlier. Wanted to test it in my project, but the update of com.fasterxml.jackson didn't work out with the rest of my project. Things got incompatible. I think my set up here is a mess. I lack the routine regarding integration. Maybe it will work out later.
Do you have a similar problem than https://github.com/redouane59/twittered/issues/427 ?
Well. There are some design decisions that need to be discussed.
Tweet interface: Obviously, API v1 and v2 are parting ways now even more. How to deal with that in the Interface
Tweet
and in the classTweetV1
with the v2 only getters likegetImpressionCount()
? Now it returns 0. Should it better throw an exception? Return -1? Changing return type to return null?Non-public: What to return on non-public metrics in the DTOs, if requested in a public context? This metrics do not exist then and just returning 0 (which is happening) is misleading. To get around it, I added some
has
-methods (e.g.hasOrganicMetrics()
). (For consistency reasons I even added them for public metrics in tweets and user.) For the single metrics I still would prefer returningInteger null
instead ofint 0
.Error management: There seems to be a whole world, where twitter returns with http 200, but has only errors in the response. This happens on insufficient access rights for non-public data. Couldn't even find docs on that on twitter. I throw
NoPermissionException
on a certain condition now, but that is highly preliminarily. Further knowledge is still missing here what can happen.Consumer key context: To determine whether requesting with consumer key for user context or app-only bearer token, a var
useConsumerKey
inRequestHelperV2
is used. But that would mean constructor explosion. So I did not add it to all constructors. I guess that is not final. Furthermore, it maybe would be nice to determine that per request, not perTwitterClient
/RequestHelperV2
.Tests: I skimmed over the tests but did not really had an idea.. I need some suggestions here what to do.