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

[NT-732] Consume category name from v1 API #1057

Merged
merged 15 commits into from
Feb 5, 2020

Conversation

justinswart
Copy link
Contributor

@justinswart justinswart commented Feb 3, 2020

📲 What

Improves deserialization of the parent category from our v1 api so that it includes the category name.

🤔 Why

This is information that we would like to include in our tracking properties.

🛠 How

  • Removed the old workaround for deserializing our Swift.Decodable Category model within our Argo.Decodable Project model.
  • Split out the v1 Category info into a Project.Category model.
  • Also added tryDecodable(_:) described in the RFC for deprecating Argo for future use.

✅ Acceptance criteria

  • No regressions introduced in current deserialization of Category (discover feed, discover filter).
  • Parent category name is deserialized correctly.

⏰ TODO

  • Add the parent category name to our tracking properties.

@justinswart justinswart added the blocked a PR that is blocked for external reasons label Feb 3, 2020
@justinswart justinswart added needs review and removed blocked a PR that is blocked for external reasons labels Feb 4, 2020
self.subcategories = try? values.decode(SubcategoryConnection.self, forKey: .subcategories)
self.totalProjectCount = try? values.decode(Int.self, forKey: .totalProjectCount)
}

/* Tries to decode the parent category from the expected GraphQL structure
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how I'm feeling about this. It seems to work but I'm going to take another look tomorrow at what would be involved to just split these two models out completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, started down a different path and don't really feel like using separate models would be significantly better. Let me know what you think @Scollaco @ifbarrera.

import Argo
import Foundation

public func tryDecodable<T: Swift.Decodable>(_ json: JSON) -> Argo.Decoded<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we planning to use this in the future? It seems that it's not being used currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good call out! I know we don't typically like to do this (and if you feel strongly, I'll remove it), but given that we have a test for it and it's very much our intention to start deprecating Argo using this function I'd like to get it into the codebase. What if I open a PR tomorrow that uses it to deprecate at least one Argo.Decodable model? 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

A new PR sounds good. I can review it. Good Job here!

Copy link
Contributor

@ifbarrera ifbarrera left a comment

Choose a reason for hiding this comment

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

So much simpler than before 🙏

@justinswart justinswart merged commit df54513 into master Feb 5, 2020
@justinswart justinswart deleted the NT-732-consume-category-name branch February 5, 2020 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants