Shopify.Pagination module #92
Conversation
|
It's possible that the |
|
Thanks @balexand for your contribution. @nsweeting any chance you have some feedback on this for us all? Shopify will remove for page based navigation in less than a month now: April 1st. Thank you both! |
|
Let me know if there's anything that I can do to help. I'm currently using the code from this PR in production and it works great. |
|
@balexand since this doesn't seem to be getting merged in I've been using your fork. I wonder if it might be worthwhile to publish a new hex package. There's still plenty of stuff that could be added, I noticed some missing fields on some resources, and there's some deprecation issues with updating products due to the inventory fields. Those should probably be removed or at least handled better for updating products. For now I've just been dumping it to a map and removing the offending keys so shopify accepts it |
|
@ProtoJazz @balexand I am a maintainer in this project, and I have to apologize for the lack of attention we have been spending on this library. Truth is (only speaking for myself here, @nsweeting might have a different oppinion) that the initial approach of having structs for each resource has lost a lot of viability since Shopify started to update their API almost every 4 months. Ever since Shopify started using versioned REST API, we have largely switched to using their GraphQL API, and I am seeing the same problems there: If you want to use any Shopify API, you better be ready to fully commit to being a Shopify developer, because they are cranking out new features and deprecations in a pace that I find disturbing. My personal opinion is that Shopify probably thinks of Shopify-App developers as basically their employees, and we have to keep up with what they dictate, but I think they are just violating the basic idea of what an API is: A lasting contract between consumer and supplier. I would like to contribute more, but I simply can not. There is no time management in the world that could give me the freedom to give you guys the attention you deserve. I spent days basically going through Shopify's REST API documentation and implementing the missing resources, trying to accommodate for their ideas in how a REST API works, coming up with half backed solutions to how we could solve it. I learned a lot, it was my entry into Elixir open-source and I would not want to miss it - but I certainly do not want (or can afford) to spend the rest of my developer life chasing Shopify's new inventions. @nsweeting by all means, I think it is time to pass the torch, find other maintainers or maybe transfer the whole project. |
|
Yeah, I suspect as shopify grows they're finding certain parts of their API just not sustainable at scale. They just grow at such a crazy pace. Thankfully after the change to the versioned api, at least theres some warning about changes and time to fix it I wouldn't be opposed to helping as a maintainer. I'm still pretty new to elixir, but I've been working with shopify everyday for the last few years and don't see any signs of stopping any time soon |
| @enforce_keys [:data, :func, :middleware, :params, :session] | ||
| defstruct @enforce_keys |
There was a problem hiding this comment.
Is there a reason to @enforce_keys if the enforced keys are the struct keys?
There was a problem hiding this comment.
@enforce_keys makes it so that it is an error to instantiate the struct without all of the fields. It has the potential to prevent bugs where a field is accidentally omitted.
It's optional and I can remove it if you'd like.
lib/shopify/enumerable.ex
Outdated
| end | ||
| end | ||
|
|
||
| def reduce(%Shopify.Enumerable{data: [h | t]} = e, {:cont, acc}, fun) do |
There was a problem hiding this comment.
Please don't use h t for head and tail. what is the head? I literally can not even give advice on what it should be, because I don't know what h is.
There was a problem hiding this comment.
I'll rename these variables to head and tail. Heads up that pattern matching using the names [h | t] and [head | tail] is a common pattern in Elixir. You can find many examples of both naming conventions in the Elixir source code, like https://github.com/elixir-lang/elixir/blob/b9209ad1ac9c879d7ea17c635a14b22aebbf2242/lib/elixir/lib/enum.ex#L2126-L2128
There was a problem hiding this comment.
h and t are pretty standard. I dont think naming them head and tail gives any more context on what the data actually is
There was a problem hiding this comment.
You are both right, and I am sorry for being unclear. I am talking from a code reading point of view. Sure, I know h and t but what the hell is it in this case!
I still hate on t and h even if there is no way to describe them further, you know why? try to select all t and h in your file and change them to tail and head - you know what I am talking about.
|
Hi @Ninigi. Thanks for your response. I can definitely relate to your difficulties with the Shopify API. It has lots of quirks and inconsistencies and they do make breaking changes at an alarming rate. I work with the Shopify API constantly at my current job so I really hope things stabilize. I would prefer to become a regular contributor to this project as opposed to completely taking it over. This could mean giving me access to merge pull-requests in the Github repo. Or it could mean that you would recognize me as a trusted contributor and be willing to merge my pull-requests with less review. In either case, I would consult with the other project members before making any significant changes. @Ninigi and @nsweeting, would you be interested in having me become a more regular contributor? Also, I'm curious to know how involved you both are interested in being with this project. We can discuss here or I can do a Zoom call if that works better for you. |
|
What do you think of removing the shopify/lib/shopify/pagination.ex Lines 31 to 35 in bd6884d |
| fun | ||
| ) | ||
|
|
||
| {:error, %Error{} = error} -> |
There was a problem hiding this comment.
Here is a 3rd case that needs to be considered:
{:error, %Response{}} e.g. Shopify fails (as the API does this relatively often) with a 500. Perhaps a retry in this case would be the best thing to do as paginating over a large collection of items can abort with high cost to re-iterate the whole collection.
There was a problem hiding this comment.
Thanks, I'll look into this. I have a busy week ahead of me so there's a chance I won't get to it until next Monday.
|
@balexand @ProtoJazz |
It hasn't been merged in. You could point at the fork this PR is based on in your mix file https://hexdocs.pm/mix/Mix.Tasks.Deps.html |
I have pointed to it, was looking for docs in how to use the new pagination |
Fixes #83. See the doc comments in the
Shopify.Paginationmodule for details and examples.