new direct message api #229

mitgard · 2018-09-25T14:03:20Z

No description provided.

codecov · 2018-09-26T11:51:17Z

Codecov Report

Merging #229 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
+ Coverage    94.2%   94.26%   +0.05%     
==========================================
  Files          75       76       +1     
  Lines        1157     1168      +11     
  Branches       11       12       +1     
==========================================
+ Hits         1090     1101      +11     
  Misses         67       67

Impacted Files	Coverage Δ
...sfregola/twitter4s/http/oauth/OAuth1Provider.scala	`100% <ø> (ø)`	⬆️
...asfregola/twitter4s/entities/enums/TweetType.scala	`100% <100%> (ø)`
...st/directmessages/TwitterDirectMessageClient.scala	`100% <100%> (ø)`	⬆️
...egola/twitter4s/http/serializers/EnumFormats.scala	`100% <100%> (ø)`	⬆️
...ola/twitter4s/http/serializers/CustomFormats.scala	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc3e35c...99d848a. Read the comment docs.

src/main/scala/com/danielasfregola/twitter4s/http/oauth/OAuth1Provider.scala

src/main/scala/com/danielasfregola/twitter4s/entities/NewDM.scala

DanielaSfregola

Hi @mitgard,
thanks for your PR!

I think we are almost there. Could you please:

clarify the reason of the changes in OAuth1Provider.scala
rename class as per comment
add return types to the public methods
Could we please add a deprecation annotation to the methods linked to the methods that Twitter is deprecating? Something like @deprecated("Twitter endpoint deprecated from 17th Sep 2018. Please use 'whatever' instead.")

.../danielasfregola/twitter4s/http/clients/rest/directmessages/TwitterDirectMessageClient.scala

DanielaSfregola · 2018-09-30T10:02:38Z

This fixes #227

mitgard · 2018-10-01T09:59:46Z

@DanielaSfregola What we do with tests for deprecated function? Exclude -depcrecation or -Xfatal-warnings from build settings or delete tests?

DanielaSfregola · 2018-10-02T18:14:12Z

I would probably temporary remove the compiler flag, rather than removing the tests

DanielaSfregola · 2018-10-02T18:17:51Z

.../danielasfregola/twitter4s/http/clients/rest/directmessages/TwitterDirectMessageClient.scala

@@ -23,6 +65,7 @@ trait TwitterDirectMessageClient {
    * @param id : The ID of the direct message.
    * @return : The direct message.
    * */
+  @deprecated("Twitter endpoint deprecated from 17th Sep 2018. Please use 'eventShow' instead.", "twitter4s 5.6")


This will be released as part of twitter4s 6.0, so can you please change the since message of the deprecated annotation?

…, fix event class

DanielaSfregola · 2018-10-07T09:40:28Z

build.sbt

@@ -74,7 +74,6 @@ lazy val standardSettings = Seq(
    "UTF-8",
    "-Xlint",
    "-deprecation",
-    "-Xfatal-warnings",


The fatal-warnings flag is too important to remove it.
If you cannot find a way to let the tests involving deprecation pass, I would rather delete them that removing the flag.

mitgard · 2018-10-16T09:27:52Z

@DanielaSfregola It seems I've finished with that. Could you please check.

DanielaSfregola

Overall looks good.
Before we can merge we need to:

use more specific types in the entity so that the API can be easier to use (e.g.: enums rather than string, time related types rather than string, etc).
[ME] double check that the changes on the token authentication don't break the other endpoints. We do have unit tests, but it is such a delicate part of the code that I'd like to do some manual testing that uses the actual Twitter API, just to be sure.

DanielaSfregola · 2018-10-18T06:35:51Z

src/main/scala/com/danielasfregola/twitter4s/entities/DirectMessageEvent.scala

@@ -0,0 +1,18 @@
+package com.danielasfregola.twitter4s.entities
+
+final case class DirectMessageEvent(`type`: String,


type should be an enum as we should know the set of possible values accepted by the Twitter API
ids general are of type Long, any particular reason they are of type String here?
timestamps should have a type consistent to the other types of timestamp in the code. Also, we may want to add a default to "now", so that people can just create the entity without providing one explicitly

DanielaSfregola · 2018-10-18T06:37:11Z

src/main/scala/com/danielasfregola/twitter4s/entities/DirectMessageEventList.scala

+import com.danielasfregola.twitter4s.entities.streaming.UserStreamingMessage
+
+final case class DirectMessageEventList(events: List[DirectMessageEvent],
+                                        apps: Option[Map[String, Apps]],


Why an Option[Map[.., ...]] and not just a Map[..., ...]?

This needs to be changed to just Map[..., ...] unless there is a specific reason to keep the optional wrapper

DanielaSfregola · 2018-10-18T06:38:16Z

src/main/scala/com/danielasfregola/twitter4s/entities/NewDirectMessageEvent.scala

+
+private[twitter4s] final case class NewDirectMessageEvent(event: NewEvent)
+
+private[twitter4s] final case class NewEvent(`type`: String = "message_create", message_create: MessageCreate)


type should be an enum

DanielaSfregola · 2018-10-18T06:41:40Z

src/main/scala/com/danielasfregola/twitter4s/http/oauth/OAuth1Provider.scala

-      if (cleanBody.nonEmpty) {
+      if (cleanBody.nonEmpty && request.entity
+            .getContentType()
+            .mediaType == MediaTypes.`application/x-www-form-urlencoded`) {


We need to check this before merging, as this could break other endpoints' authentication.

May be we return Map() if we cannot calculate Map

build.sbt

id to Long timestamp to LocalDateTime

mitgard · 2018-11-12T06:58:27Z

@DanielaSfregola, Could you please check. I fixed you comments.

DanielaSfregola · 2018-11-13T19:36:16Z

src/main/scala/com/danielasfregola/twitter4s/entities/DirectMessageEvent.scala

+
+final case class DirectMessageEvent(`type`: TweetType.Value = TweetType.messageCreate,
+                                    id: DirectMessageId,
+                                    created_timestamp: LocalDateTime,


created_timestamp cannot be a local date as we need to consider timezones as well. I suggest to change it to Instant and to add a default of Instant.now.

DanielaSfregola · 2018-11-13T19:40:14Z

Hi @mitgard,
thanks for your awesome work! We are almost there. The only things still missing:

get rid of Option[Map[..., ...]] or convince me why we need to keep the distintion between no map and empty map
change created_timestamp form LocalDateTime to Instant.

I know you have been working on this PR for quite some time now, so I am happy to merge it as it and prepare a patch for it if you no longer have the time to work on it.

Cheers,
Daniela

mitgard · 2018-11-19T09:01:00Z

Hi @mitgard,
thanks for your awesome work! We are almost there. The only things still missing:

get rid of Option[Map[..., ...]] or convince me why we need to keep the distintion between no map and empty map

change created_timestamp form LocalDateTime to Instant.

I know you have been working on this PR for quite some time now, so I am happy to merge it as it and prepare a patch for it if you no longer have the time to work on it.

Cheers,
Daniela

Hi @DanielaSfregola, sorry for my long time patch. I've fixed last comment.

DanielaSfregola

LGTM!

…ssages #229 Minor changes and clean up

mitgard added 5 commits September 24, 2018 17:27

Fix new direct message endpoints

b4b490b

Fix new direct message endpoints

86acb32

Fix new direct message endpoints

6abfa6b

Fix new direct message endpoints

255d4d1

Fix test passed

fe1bd60

mitgard added 2 commits September 26, 2018 16:45

Add some test

cb4c71d

Fixed tests

6c385b6

DanielaSfregola reviewed Sep 30, 2018

View reviewed changes

src/main/scala/com/danielasfregola/twitter4s/http/oauth/OAuth1Provider.scala Show resolved Hide resolved

DanielaSfregola reviewed Sep 30, 2018

View reviewed changes

src/main/scala/com/danielasfregola/twitter4s/entities/NewDM.scala Outdated Show resolved Hide resolved

DanielaSfregola suggested changes Sep 30, 2018

View reviewed changes

mitgard added 3 commits October 1, 2018 10:53

Some fix by comments

c0beb29

Scalafmt fix

896bdc7

Depreceated annotation

6623013

DanielaSfregola reviewed Oct 2, 2018

View reviewed changes

DanielaSfregola changed the title ~~fix direct message api~~ new direct message api Oct 2, 2018

mitgard added 4 commits October 3, 2018 18:46

Delete deprecation flag, edit lib version tag in deprecation function…

bac3c5b

…, fix event class

Delete deprecation flag, edit lib version tag in deprecation function…

a5ff039

…, fix event class

Delete deprecation flag, edit lib version tag in deprecation function…

8634ff3

…, fix event class

delete -Xfatal-warnings

82a575c

DanielaSfregola reviewed Oct 7, 2018

View reviewed changes

mitgard added 2 commits October 8, 2018 10:34

Turn off -deprecation

349bf5f

Return -Xfatal-warnings

cafb82f

DanielaSfregola suggested changes Oct 18, 2018

View reviewed changes

DanielaSfregola reviewed Oct 18, 2018

View reviewed changes

build.sbt Show resolved Hide resolved

Replace type to Enum type

7a05f8a

id to Long timestamp to LocalDateTime

mitgard added 4 commits November 8, 2018 17:44

syntax fix

a677a7f

jint to long

3ad4586

jint to long

00e9b9f

fix formatting

6fb2cea

DanielaSfregola reviewed Nov 13, 2018

View reviewed changes

Change LocalDateTime to Instant, Option[Map[..,..]] to Map[..,..].

99d848a

DanielaSfregola approved these changes Nov 25, 2018

View reviewed changes

DanielaSfregola added the ready-to-merge label Nov 25, 2018

DanielaSfregola merged commit 34f6c59 into DanielaSfregola:master Dec 1, 2018

DanielaSfregola added a commit that referenced this pull request Dec 1, 2018

#229 Minor changes and clean up

99fe8b3

DanielaSfregola removed the ready-to-merge label Dec 1, 2018

DanielaSfregola added this to the 6.0 milestone Dec 1, 2018

This was referenced Dec 1, 2018

#229 Minor changes and clean up #232

Merged

Deprecated API on direct messages #176

Closed

DanielaSfregola added a commit that referenced this pull request Dec 1, 2018

Merge pull request #232 from DanielaSfregola/#229_follow_up_direct_me…

cc5ffbc

…ssages #229 Minor changes and clean up

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new direct message api #229

new direct message api #229

mitgard commented Sep 25, 2018

codecov bot commented Sep 26, 2018 •

edited

Loading

DanielaSfregola left a comment

DanielaSfregola commented Sep 30, 2018

mitgard commented Oct 1, 2018 •

edited

Loading

DanielaSfregola commented Oct 2, 2018

DanielaSfregola Oct 2, 2018 •

edited

Loading

DanielaSfregola Oct 7, 2018

mitgard commented Oct 16, 2018

DanielaSfregola left a comment

DanielaSfregola Oct 18, 2018

mitgard Nov 9, 2018

DanielaSfregola Oct 18, 2018

mitgard Nov 9, 2018

DanielaSfregola Nov 13, 2018 •

edited

Loading

DanielaSfregola Oct 18, 2018

mitgard Nov 9, 2018

DanielaSfregola Oct 18, 2018

mitgard Nov 9, 2018

mitgard commented Nov 12, 2018

DanielaSfregola Nov 13, 2018

DanielaSfregola commented Nov 13, 2018

mitgard commented Nov 19, 2018

DanielaSfregola left a comment

		@@ -0,0 +1,18 @@
		package com.danielasfregola.twitter4s.entities

		final case class DirectMessageEvent(`type`: String,


		private[twitter4s] final case class NewDirectMessageEvent(event: NewEvent)

		private[twitter4s] final case class NewEvent(`type`: String = "message_create", message_create: MessageCreate)

new direct message api #229

new direct message api #229

Conversation

mitgard commented Sep 25, 2018

codecov bot commented Sep 26, 2018 • edited Loading

Codecov Report

DanielaSfregola left a comment

Choose a reason for hiding this comment

DanielaSfregola commented Sep 30, 2018

mitgard commented Oct 1, 2018 • edited Loading

DanielaSfregola commented Oct 2, 2018

DanielaSfregola Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitgard commented Oct 16, 2018

DanielaSfregola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielaSfregola Nov 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitgard commented Nov 12, 2018

Choose a reason for hiding this comment

DanielaSfregola commented Nov 13, 2018

mitgard commented Nov 19, 2018

DanielaSfregola left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 26, 2018 •

edited

Loading

mitgard commented Oct 1, 2018 •

edited

Loading

DanielaSfregola Oct 2, 2018 •

edited

Loading

DanielaSfregola Nov 13, 2018 •

edited

Loading