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

Add contentMediaType for variants object #47

Merged
merged 4 commits into from Aug 15, 2022

Conversation

emin-grbo
Copy link
Contributor

Unfortunately I was not able to find proper spec in the Twitter API for this before, and only way to really test this is to have it in the package and use it.

Or could we somehow test this in the demo app @daneden ?

As for this PR, I presumed that mediaType and contentType are the same, however from a bit of testing it turns you types slightly differ in strings, so we needed a new enum to handle that.

@daneden
Copy link
Owner

daneden commented Aug 12, 2022

This is my bad; I should really have asked for tests in your other PR(s)!

If you check out the tests folder you should be able to copy one for testing these changes by making sure accessing properties works as expected. It's based on the Twitter v2 OpenAPI spec which they use for their own playground development and some docs generation.

@emin-grbo
Copy link
Contributor Author

Oh got it. Will look into that today and add them in 🙌

@emin-grbo
Copy link
Contributor Author

So not really sure how to test this inside an array tbh. I guess you were referring to one of these tests?

  func testAddListMember() async throws {
    let addListMemberResult = try await TwiftTests.userAuthClient.addListMember("0", to: "0")
    XCTAssertTrue(addListMemberResult.data.isMember)
  }

All I can see that I can do to test if the types are there is this. But can only check if they are accessible from within so not sure if that helps testing the public security access level?

final class TwiftTypesTests: XCTestCase {
  func testMediaVariantsBitrateAccessible() async throws {
    let getTweetResult = try await TwiftTests.userAuthClient.getTweet("0")
    XCTAssertNil(getTweetResult.includes?.media?.first?.variants?.first?.bitRate)
  }
  
  func testMediaVariantsContentTypeAccessible() async throws {
    let getTweetResult = try await TwiftTests.userAuthClient.getTweet("0")
    XCTAssertNil(getTweetResult.includes?.media?.first?.variants?.first?.contentType)
  }
  
  func testMediaVariantsUrlAccessible() async throws {
    let getTweetResult = try await TwiftTests.userAuthClient.getTweet("0")
    XCTAssertNil(getTweetResult.includes?.media?.first?.variants?.first?.url)
  }
  
}

Or did you mean for me to instantiate a mock media object?

@daneden
Copy link
Owner

daneden commented Aug 14, 2022

Oh I see, you mean testing the access levels! Yeah I think since Twift is annotated with @testable in the test suite it will ignore access levels. I'd recommend testing changes related to access levels via the demo app.

@@ -95,7 +104,7 @@ extension Media {
public var bitRate: Int?

/// Type of media
public var contentType: MediaType
public var contentType: ContentMediaType
Copy link
Owner

Choose a reason for hiding this comment

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

I think this should probably just conform to UTType!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow...first time I saw this existed 😅 Seems convenient. It covers all "standard" extensions/types basically?

@emin-grbo
Copy link
Contributor Author

Oh I see, you mean testing the access levels! Yeah I think since Twift is annotated with @testable in the test suite it will ignore access levels. I'd recommend testing changes related to access levels via the demo app.

Right! Inside the demo app it is 👌 to test, but not sure how I can write actual tests to see if the data is there?

Another note, just tested it with the latest UTType change, it doesn't work. But it does with the previous ContentMediaType 🤷‍♂️
I will read more about UTType in general, maybe I wrongly assumed it would work without anything more custom.

@daneden
Copy link
Owner

daneden commented Aug 15, 2022

@roblack yeah I noticed UTType doesn't work as expected here; I think because the decoding initialiser expects an identifier(e.g. "public.mpeg4") instead of a MIME type (e.g. "video/mp4"). It may make sense to just decode contentType as a String to account for arbitrary future MIME types that may be returned. Then library users can do with it whatever they need to (such as casting to UTType to do their own content decoding or streaming)

Sources/Types+Media.swift Outdated Show resolved Hide resolved
Sources/Types+Media.swift Outdated Show resolved Hide resolved
@daneden
Copy link
Owner

daneden commented Aug 15, 2022

not sure how I can write actual tests to see if the data is there?

Unfortunately it looks like Twitter's OpenAPI spec doesn't make this as easy as I had hoped either.

Basically, it only synthesises some types correctly. Here's an example payload from GET /2/tweets/{id} based on their OpenAPI spec:

{
  "data": {
    "author_id": "2244994945",
    "created_at": "Wed Jan 06 18:40:40 +0000 2021",
    "id": "1346889436626259968",
    "text": "Learn how to use the user Tweet timeline and user mention timeline endpoints in the Twitter API v2 to explore Tweet\\u2026 https:\\/\\/t.co\\/56a0vZUx7i"
  },
  "errors": [
    {
      "detail": "string",
      "status": 0,
      "title": "string",
      "type": "string"
    }
  ],
  "includes": {
    "media": [
      {
        "height": 0,
        "media_key": "string",
        "type": "string",
        "width": 0
      }
    ],
    "places": [
      {
        "contained_within": [
          "f7eb2fa2fea288b1"
        ],
        "country": "United States",
        "country_code": "US",
        "full_name": "Lakewood, CO",
        "geo": {
          "bbox": [
            -105.193475,
            39.60973,
            -105.053164,
            39.761974
          ],
          "geometry": {
            "coordinates": [
              -105.18816086351444,
              40.247749999999996
            ],
            "type": "Point"
          },
          "properties": {},
          "type": "Feature"
        },
        "id": "f7eb2fa2fea288b1",
        "name": "Lakewood",
        "place_type": "city"
      }
    ],
    "polls": [
      {
        "duration_minutes": 5,
        "end_datetime": "2019-08-24T14:15:22Z",
        "id": "1365059861688410112",
        "options": [
          {
            "label": "string",
            "position": 0,
            "votes": 0
          },
          {
            "label": "string",
            "position": 0,
            "votes": 0
          }
        ],
        "voting_status": "open"
      }
    ],
    "topics": [
      {
        "description": "All about technology",
        "id": "string",
        "name": "Technology"
      }
    ],
    "tweets": [
      {
        "author_id": "2244994945",
        "created_at": "Wed Jan 06 18:40:40 +0000 2021",
        "id": "1346889436626259968",
        "text": "Learn how to use the user Tweet timeline and user mention timeline endpoints in the Twitter API v2 to explore Tweet\\u2026 https:\\/\\/t.co\\/56a0vZUx7i"
      }
    ],
    "users": [
      {
        "created_at": "2013-12-14T04:35:55Z",
        "id": "2244994945",
        "name": "Twitter Dev",
        "protected": false,
        "username": "TwitterDev"
      }
    ]
  }
}

As you can see in the above includes.media example, not only does the OpenAPI spec prevent us from requesting additional fields (such as variants), but it also doesn't have correct types for e.g. the type field. Short of providing feedback to the Twitter Developer team, the best approach is probably a combination of tests for supported structures such as Tweet as well as some property testing in the demo app.

@daneden daneden merged commit 4f375f5 into daneden:main Aug 15, 2022
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