stocky37 commented 3 years ago

Closes #83

Moves the regex interpolation of the url to category/id to lazy initialized values in the data classes rather than in the serializer/deserializer.

This does technically break backwards compatibility as the constructors of NamedApiResource and ApiResource change, but I thought this shouldn't matter too much as most people would be just using the values already constructed from the api call.

I also tried a bunch of different ways to try and get it to work without the adapters (seeing as they shouldn't be required anymore) but couldn't get anything working due to the way Gson have implemented the reflection.

codecov[bot] commented 3 years ago

Codecov Report

Merging #84 (58701eb) into master (42a67f2) will increase coverage by 0.07%. The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master      #84      +/-   ##
============================================
+ Coverage     65.57%   65.64%   +0.07%     
  Complexity      106      106              
============================================
  Files            19       19              
  Lines           883      885       +2     
  Branches          3        3              
============================================
+ Hits            579      581       +2     
  Misses          303      303              
  Partials          1        1

Impacted Files	Coverage Δ	Complexity Δ
...in/me/sargunvohra/lib/pokekotlin/model/resource.kt	`83.33% <100.00%> (+6.86%)`	`0.00 <0.00> (ø)`
...lin/me/sargunvohra/lib/pokekotlin/util/adapters.kt	`100.00% <100.00%> (ø)`	`0.00 <0.00> (ø)`

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 42a67f2...6c7e5f8. Read the comment docs.

Naramsim commented 3 years ago

@sargunv, I'm not able to review this code and I'm a bit worried about the backward-compatibility that @stocky37 mentioned.

Could you give us a quick review? 🙇‍♂️

stocky37 commented 3 years ago

...I'm a bit worried about the backward-compatibility that @stocky37 mentioned.

If you're worried about the backwards compatibility I'll take a look and if there's anything I can do to keep it.

sargunv commented 3 years ago

Will take a look sometime in the next couple of days. Re backwards compat, feel free to bump the major version to communicate that to users.

stocky37 commented 3 years ago

I think I found a good solution. An alternate constructor with the old signature should suffice, then creating a url from the category and id. The only real difference now is that technically the url property is being used to compare *ApiResource objects even if they are instantiated with categry/id. Reverted the tests back to their original state to make sure I was keeping backwards compat. Also makes the change way smaller to grep.

The only time this would break compared to the old implementation is if they were comparing manually instantiated objects with those returned from an API, and the API returned something other than /$category/$id/ - but since this is not the case I think it's a non-issue.

stocky37 commented 3 years ago

Thanks for the merge. Anything you need me to do to get this released?

sargunv commented 3 years ago

Nah, it's on my to-do list for this weekend.

Meanwhile are you able to use it through jitpack?

stocky37 commented 3 years ago

No worries, no rush. I'll give jitpack a try now. Do you have the coordinates onhand?

stocky37 commented 3 years ago

Wait, they would be the same just on the jitpack repo, right?

sargunv commented 3 years ago

Ah damn, I just tried kicking off a jitpack build and it failed. It's been a while since I've used jitpack.

stocky37 commented 3 years ago

[ERROR] Failed to execute goal on project pokeapi-core: Could not resolve dependencies for project ... Failure to find me.sargunvohra.lib:pokekotlin:jar:2.3.1 in https://jitpack.io was cached in the local repository, resolution will not be reattempted until the update interval of jitpack.io has elapsed or updates are forced

sargunv commented 3 years ago

Jitpack coordinates are different; you can see the details here: https://jitpack.io/#PokeAPI/pokekotlin

Unfortunately it doesn't appear to be working anymore. Will investigate that later and try to publish this release today.

sargunv commented 3 years ago

2.4.0 should be out on Bintray now. Lmk if you have any trouble with it.

I did just notice Bintray is going away soon, so we'll need to find a new package repository to publish future versions to (Github Packages?).

PokeAPI / pokekotlin

Refactor ApiResource models to use lazy vals for category & id #84

Codecov Report