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 **params for all collection endpoints #70

Conversation

timchaston
Copy link
Contributor

Hi,

I realised I needed to be able to use the 'limit' and 'offset' pagination parameters for collections, otherwise I was only able to pull down the first 20 of any particular thing (specifically I needed to be able to access more than 20 custom fields).

I've added double-splat params to all the collection endpoint methods so that they can be supplied, as can anything else, rather than adding an explicit and specific arguments for them.

Let me know if you prefer another approach.

Also, I haven't written any tests for 'limit' and 'offset' (well, I did, but only for one endpoint, and it was hacky, so I figured I'd be better off getting feedback before spending the time on writing them for more endpoints). Let me know if you have any suggestions/preferences for tests. Do the pagination parameters need to be tested for every one of the collection endpoints? Also, do we need to test large numbers of items, or is 2 sufficient, do you think? Currently there's no easy way to generate an arbitrary number of different users set up. I see factory_bot is in there, but not set up for Contacts at least (in my hacky test, I just inserted counter into the email address to create 50 Contacts).

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 15160 lines exceeds the maximum allowed for the inline comments feature.

@timchaston timchaston force-pushed the v3-add-splat-params-to-collection-endpoints branch from d400b06 to 430bf49 Compare July 14, 2020 05:52
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 34454 lines exceeds the maximum allowed for the inline comments feature.

@timchaston timchaston force-pushed the v3-add-splat-params-to-collection-endpoints branch from 430bf49 to 737623e Compare July 15, 2020 05:16
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 34454 lines exceeds the maximum allowed for the inline comments feature.

@timchaston timchaston force-pushed the v3-add-splat-params-to-collection-endpoints branch 3 times, most recently from 80e14a1 to 3786ade Compare July 15, 2020 07:09
@timchaston
Copy link
Contributor Author

Fixes made for bugs and style.

Rubocop and codeclimate are still unhappy, but with issues that are present on master. Thought I should mention, also, that rubocop passes just fine locally, so I'm not quite sure why it's failing here.

@mhenrixon
Copy link
Owner

Rubocop and codeclimate are still unhappy, but with issues that are present on master. Thought I should mention, also, that rubocop passes just fine locally, so I'm not quite sure why it's failing here.

Because on github actions a newer version is installed of rubocop :)

@timchaston
Copy link
Contributor Author

Because on github actions a newer version is installed of rubocop :)

I'd already updated rubocop itself to v0.88.0, but yeah, I had neglected to update all the other 'rubocop-*' gems. Got the offences after upgrading them. Cheers!

Made fixes and chucked them into a new PR ( #72 ), since they're unrelated to the work in this one.

@mhenrixon
Copy link
Owner

You can go ahead and merge those changes into this branch now and I'll get this merged!

Copy link
Owner

@mhenrixon mhenrixon left a comment

Choose a reason for hiding this comment

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

This looks much better! 👍

@timchaston timchaston force-pushed the v3-add-splat-params-to-collection-endpoints branch 2 times, most recently from 213c242 to dfb1813 Compare July 17, 2020 01:48
@timchaston timchaston force-pushed the v3-add-splat-params-to-collection-endpoints branch from dfb1813 to 5b19db2 Compare July 17, 2020 02:06
@codeclimate
Copy link

codeclimate bot commented Jul 17, 2020

Code Climate has analyzed commit 5b19db2 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

View more on Code Climate.

@timchaston
Copy link
Contributor Author

timchaston commented Jul 17, 2020

Merged in from master, so rubocop is now happy, but codeclimate is still complaining about duplication.

There's not really anything, to my mind, that should be done to address it in this ticket. It'd require either:

  • making arbitrary changes (e.g. change one of the params[:search] = search to be params.merge({search: search}, or possibly changing the order of methods?) which would only be achieving a degradation in consistency of style (in fact that would be the sole aim)

  • an overhaul where the files would be combined or make common calls to a newly created third file - in either case it'd be creating another set of common methods to which different string literals are passed by the existing methods, which is just totally pointless, creates more code, that is less readable, degrades the architectural consistency, and makes the whole thing more brittle to changes

Are there any options I'm missing? Do you have any suggestions? Or can you just do a manual override?

@mhenrixon mhenrixon merged commit aff6b05 into mhenrixon:master Jul 17, 2020
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.

None yet

2 participants