chrisdhaan / CDYelpFusionKit

An extensive Swift wrapper for the Yelp Fusion API.
MIT License
54 stars 18 forks source link

Nonconsistent Rezzo Names #8

Open elope-developers opened 6 years ago

elope-developers commented 6 years ago

What did you do?

ℹ Try and call categories/cost by enum string value for "beer_and_wine" [CDYelpBusinessCategoryFilter(rawValue: "beer_and_wine")!]

What did you expect to happen?

ℹ Have algorithm Return "beer_and_wine" via case beer_and_wine

What happened instead?

ℹ The CDYelpEnums class could not find a case with 'beer_and_wine' because it was named: beerWineAndSpirits

It appears that there are multiple instances of this happening. So far I've made customizations to several similar case statements. I hate even bother with this as an issue because it's such a simple fix on our end, but I can only imagine as more and more people begin to use this awesome project people will end up confused. I'm am all for camel-cased variable names, but I think matching the variable names with the value they are equal to in the official Yelp backend may be best.

CDYelpFusionKit Environment

CDYelpFusionKit version: lateset version
Xcode version: 9.2
Swift version: currently swift 4 (also noticible in swift 3)
Platform(s) running CDYelpFusionKit: iOS 10+
macOS version running Xcode: HS 10.13.2

Demo Project

ℹ Please link to or upload a project we can download that reproduces the issue. Here is an example search written in swift 4: CDYelpFusionKitManager.shared.apiClient.searchBusinesses( byTerm: time_of_day_preset, location: nil, latitude: self.lat, longitude: self.long, radius: 10000, categories: [CDYelpBusinessCategoryFilter(rawValue: "beer_and_wine")!], locale: .english_unitedStates, limit: 25, offset: 0, sortBy: .distance, priceTiers: nil, openNow: true, openAt: nil, attributes: nil) { (response) in if let response = response, let businesses = response.businesses, businesses.count > 0{ for biz in businesses{ print(biz) } } }

CDYelpFusionKitManager.shared.apiClient.searchBusinesses( byTerm: time_of_day_preset, location: nil, latitude: self.lat, longitude: self.long, radius: 10000, categories: [CDYelpBusinessCategoryFilter(rawValue: "beer_and_wine")!], locale: .english_unitedStates, limit: 25, offset: 0, sortBy: .distance, priceTiers: nil, openNow: true, openAt: nil, attributes: nil) { (response) in if let response = response, let businesses = response.businesses, businesses.count > 0{
for biz in businesses{ print(biz) } } }

This function works with case beer_and_wine = "beer_and_wine" but not case beerWineAndSpirits = "beer_and_wine" I apologize for the lack of tabs. I couldn't get the spacing to work

chrisdhaan commented 6 years ago

@Linger-Software why not just do categories: [.beerAndWine] opposed to creating an object? That's how I intended that to be used. That's also how it is displayed in the documentation in the README.

elope-developers commented 6 years ago

We are calling data from the server stored as a string :/ and because the Yelp API on the web and Android use the actual API keys we saw no need to modify the keys on our end. However this library uses custom ones, so we've tried to modify the keys we need, but eventually, we'll be looking at having to manage hundreds as we continue adding more and more.

To get a live demo of how relatively complex things can get you can download our app @ https://www.rezzo.xyz

chrisdhaan commented 6 years ago

I still don't understand why you need to modify the keys. Web and Android have nothing to do with this library. Autocomplete lists all the categories just fine. This is not an issue with the framework. This is your personal preference. If you don't want the enum in camel case then fork the repo and update that class.

davecom commented 6 years ago

Is the issue that the category names don't match the official category names, as outlined here: https://www.yelp.com/developers/documentation/v3/category_list

I guess I'm not 100% understanding the issue either.

elope-developers commented 6 years ago

We use your library on ios.

For starters .beerAndWine doesn't exist. You have named it .beerAndWineSpirits (no idea why lol) as well with many other categories.

An easy example. For applications that do a dynamic search based on data from a server (specifically yelp user data or whatever) and the category key "beer_and_wine" is passed back, whenever we run a query, we would pass that key through the [CDYelpBusinessCategoryFilter(rawValue: data[0])! (where data 0 = "beer_and_wine" or "foodtruck" or "any category name"). Because swift can't find a case that matches ".beer_and_wine" it will fail, because the library is 'beerAndWineSpirits'.

What Swift wants: case beer_and_wine = "beer_and_wine"

What CSYFK has: case beerWineAndSpirits = "beer_and_wine"

This is the case for many categories. If one builds an app for just iOS then there's no real issue, but for cross-platform development, having all keys match the OFFICIAL Yelp API keys is ideal.

On Fri, Feb 2, 2018 at 10:08 AM, Christopher de Haan < notifications@github.com> wrote:

I still don't understand why You need to modify the keys. Web and Android have nothing to do with this library. Autocomplete should list all the categories just fine. This is not an issue with the framework. This is your personal preference. If you don't want the enum in camel case then fork the repo and update that class.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/chrisdhaan/CDYelpFusionKit/issues/8#issuecomment-362660706, or mute the thread https://github.com/notifications/unsubscribe-auth/AauFV4EFlvfUQvRf9gexA-UDUQvCc4uzks5tQ08pgaJpZM4Rq1dX .

davecom commented 6 years ago

What CSYFK has: case beerWineAndSpirits = "beer_and_wine"

@Linger-Software I still don't understand why your specific example is an issue. As long as the rawValue (the literal String on the right hand side) is the same as the one on the other platforms you should be able to convert back-and-forth to the enum. What am I missing?

elope-developers commented 6 years ago

For some reason the raw value is trying to match the variable with the case enum name instead of the actual value.

I.e. the enum fruitsAndVeggies = 'markets'. When we call for the .raw value "markets", swift will value and say: "no enum matching [markets] found"

chrisdhaan commented 6 years ago

@Linger-Software this library was built with iOS in mind, not cross platform development. Use a different library to solve your cross platform development needs or fork this repo and update the categories yourself.

davecom commented 6 years ago

the enum fruitsAndVeggies = 'markets'. When we call for the .raw value "markets", swift will value and say: "no enum matching [markets] found"

That sounds like a very bizarre bug but should have nothing to do with this library. That sounds like either a bug in Swift or in your code.

elope-developers commented 6 years ago

Really it's a great library and very fast, just a couple of the developers working on this project have pointed out the weirdly renamed enums causing trouble for the Rezzo (iOS) counterpart. We don't have an issue with the library as a whole. We can definitely fork off, but we wanted to make the bug aware of Swift mishandling "raw value." We left some test code to try it on your end