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 firefish flavour #217

Merged
merged 10 commits into from
Nov 28, 2023
Merged

add firefish flavour #217

merged 10 commits into from
Nov 28, 2023

Conversation

technicat
Copy link
Contributor

Didn't see doc of Mastodon compatibility so checked the source and the web UI.

Didn't see doc of Mastodon compatibility so checked the source and the web UI.
firefish doesn't always return it, particularly in reblogs
not always returned by firefish
@kkostov
Copy link
Contributor

kkostov commented Nov 27, 2023

Awesome! @technicat Have you been able to test it against a Firefish server? Do you know which version was it?

@technicat
Copy link
Contributor Author

Awesome! @technicat Have you been able to test it against a Firefish server? Do you know which version was it?

Running against the firefish.social. Found at least a few glitches, fixed a couple that break my timelines (firefish doesn't always return statuses_count in accounts and emojis in polls, so I switched them to optional). If you want to wait on a merge, I'll push them later today. Btw, I opened up testflight invites for my app, you can try it out at fedicat.com

IMG_5109
IMG_5108

swift-atomics, swift-collections, and swift-nio
@kkostov
Copy link
Contributor

kkostov commented Nov 27, 2023

@technicat I can definitely wait a bit if you want to push some more refinements. Very much appreciated! And... downloading the TestFlight now 🚀 🤩

swift-atomics, swift-collection, and swift-nio
@@ -127,7 +127,7 @@ public extension TootClient {

extension TootFeature {

/// Ability to query trends
/// Ability to block domains
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@technicat
Copy link
Contributor Author

@technicat I can definitely wait a bit if you want to push some more refinements. Very much appreciated! And... downloading the TestFlight now 🚀 🤩

Done! (for now). Also updated packages in tootsdk and the example when running the tests.

@kkostov
Copy link
Contributor

kkostov commented Nov 27, 2023

@technicat What do you think about using default values for these fields, so that the change is not breaking. e.g. public var emojis: [Emoji]? becomes public var emojis: [Emoji] = [] and same for postsCount becomes a var with a default value of 0 public var postsCount: Int = 0.

In addition to existing apps not having to make code changes, I see it as an advantage that someone using the library doesn't need to readup on firefish before accessing these fields.

@technicat
Copy link
Contributor Author

technicat commented Nov 27, 2023

@technicat What do you think about using default values for these fields, so that the change is not breaking. e.g. public var emojis: [Emoji]? becomes public var emojis: [Emoji] = [] and same for postsCount becomes a var with a default value of 0 public var postsCount: Int = 0.

In addition to existing apps not having to make code changes, I see it as an advantage that someone using the library doesn't need to readup on firefish before accessing these fields.

Yeah, it'd be nice to be able to rely on the Mastodon spec! Won't this also require adding a Decoder to handle the missing data? It's hard to test right now because firefish.social is returning internal server errors, so I reverted the optional change, figure out what to do later.

update: firefish.social timelines are back up, so was able try this, but it's back to the original error, going to need some custom deserialization

Key 'CodingKeys(stringValue: "statusesCount", intValue: nil)' not found:No value associated with key CodingKeys(stringValue: "statusesCount", intValue: nil) ("statusesCount").
codingPath:[_JSONKey(stringValue: "Index 10", intValue: 10), CodingKeys(stringValue: "reblog", intValue: nil), CodingKeys(stringValue: "account", intValue: nil)]

@technicat
Copy link
Contributor Author

now using customer Decoders to supply default values for the missing values. My only minor concern is that don't actually want to read and use 0 for Account.postsCount as a substitute for nil (because maybe it's not zero, just not supplied) but I presume firefish is deliberately not supplying it in cases like reblogger info on the assumption you're not going to display that info there.

@davidgarywood
Copy link
Contributor

I think we've landed on a reasonable solution over all here - don't make breaking changes. Firefish seems to be the odd one out and shouldn't drive this across the entire package.

I'm wondering if we should be ripping this bandaid off at some point though, and actually making some things optional. This is a bigger discussion though - if I get any clear thoughts on it I'll kick off an issue and tag folks in.

Happy to approve this, and thanks @technicat - it's brilliant to be able to support the 'fish! 🔥🐟

Copy link
Contributor

@davidgarywood davidgarywood left a comment

Choose a reason for hiding this comment

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

🔥🐟 - Yay!

@davidgarywood davidgarywood merged commit fe05c74 into TootSDK:main Nov 28, 2023
4 checks passed
@technicat
Copy link
Contributor Author

technicat commented Nov 28, 2023

I think we've landed on a reasonable solution over all here - don't make breaking changes. Firefish seems to be the odd one out and shouldn't drive this across the entire package.

I'm wondering if we should be ripping this bandaid off at some point though, and actually making some things optional. This is a bigger discussion though - if I get any clear thoughts on it I'll kick off an issue and tag folks in.

We could also go the other direction and adhere more tightly to the Mastodon spec, for example Account.username is optional but not listed as optional in the Mastodon spec, so we could initialize it with an empty string in the Decoder if it's not returned in the JSON.

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.

3 participants