added headers extra #141

ddnexus · 2019-03-10T08:18:57Z

Add RFC-8288 compilant HTTP response headers (and other helpers) useful for API pagination.

No need for an extra dependency
No need to learn yet another interface
It saves quite a lot of memory and CPU
It works with any pagy object (including Pagy::Countless) regardless the type of collection
It offers more explicit flexibility and simplicity

ddnexus · 2019-03-10T08:32:56Z

@grosser, @bquorning, @gamafranco, @excid3, @cseelus, @MarieS1415, @NARKOZ, @twnaing...

You feedback would be appreciated!
Thanks

grosser · 2019-03-10T14:36:20Z

docs/extras.md

-| `semantic`            | Add nav, responsive and compact helpers for the Semantic UI CSS [pagination component](https://semantic-ui.com/collections/menu.html)                  | [semantic.rb](https://github.com/ddnexus/pagy/blob/master/lib/pagy/extras/semantic.rb), [documentation](extras/semantic.md)                                  |
-| `support`             | Extra support for features like: incremental, infinite, auto-scroll pagination                                                                         | [support.rb](https://github.com/ddnexus/pagy/blob/master/lib/pagy/extras/support.rb), [documentation](extras/support.md)                                     |
-| `trim`                | Remove the `page=1` param from links                                                                                                                   | [trim.rb](https://github.com/ddnexus/pagy/blob/master/lib/pagy/extras/trim.rb), [documentation](extras/trim.md)                                              |
+| Extra                 | Description                                                                                                                               | Links                                                                                                                                                        |


could avoid the giant diff / make blame simler by not aligning the |

true, but a non-aligned source makes me sick :)

grosser · 2019-03-10T14:38:35Z

docs/extras/headers.md

+  pagy, records = pagy(collection, vars) # any pagy_* backend constructor works
+  pagy_headers_merge(pagy)
+  render json: records
+end


could be more universal to do

def render(*args, &block) pagy_headers_merge(@pagy) if @pagy super end

true! This one is a nice minimalist way to just add the headers automatically that works also with existing code. The other one is more a suggestion about encapsulating/decluttering. I will use both suggestions. Thanks

grosser · 2019-03-10T14:39:04Z

docs/extras/headers.md

+
+## Files
+
+This extra is composed of 1 small file:


this comment is kinda redundant

true. I will remove the stupid comments from all the extras and leave only the links to the files. Thanks

grosser · 2019-03-10T14:43:44Z

lib/pagy/extras/headers.rb

+
+    def pagy_headers(pagy)
+      hash = pagy_headers_hash(pagy)
+      { 'Links'    => hash[:links].map{|rel, link| %(<#{link}>; rel="#{rel}")}.join(', ') }.tap do |h|


more readable/performant to not use .tap

probably... I will try

grosser · 2019-03-10T14:44:03Z

lib/pagy/extras/headers.rb

+      rels      = { first: 1, prev: pagy.prev, next: pagy.next }.tap{ |r| r[:last] = pagy.last unless countless }
+      a, b      = pagy_url_for(Frontend::MARKER, pagy, :url).split(Frontend::MARKER, 2)
+      links     = Hash[rels.map{|rel, n|[rel, "#{a}#{n}#{b}"] if n}.compact]
+      { links: links }.tap do |h|


same here, prefer no tap

grosser · 2019-03-10T14:47:40Z

lib/pagy/extras/headers.rb

+    def pagy_headers_hash(pagy)
+      countless = defined?(Pagy::Countless) && pagy.is_a?(Pagy::Countless)
+      rels      = { first: 1, prev: pagy.prev, next: pagy.next }.tap{ |r| r[:last] = pagy.last unless countless }
+      a, b      = pagy_url_for(Frontend::MARKER, pagy, :url).split(Frontend::MARKER, 2)


got some more descriptive names than a/b ?

like part_a and part_b? 😄

was hoping they have a meaning like url/query "MARKER" is also vague :D

well, MARKER means... it's just a MARKER 😄 ... used to split the string into 2 parts and wrap them around the page number. That was faster than using sub to replace MARKER with the page number (at least in pagy_link_proc), but the 2 parts are just 2 strings without any logical meaning. It's difficult to give them a meaningful name.

grosser · 2019-03-10T14:50:35Z

lib/pagy/extras/headers.rb

+      countless = defined?(Pagy::Countless) && pagy.is_a?(Pagy::Countless)
+      rels      = { first: 1, prev: pagy.prev, next: pagy.next }.tap{ |r| r[:last] = pagy.last unless countless }
+      a, b      = pagy_url_for(Frontend::MARKER, pagy, :url).split(Frontend::MARKER, 2)
+      links     = Hash[rels.map{|rel, n|[rel, "#{a}#{n}#{b}"] if n}.compact]


if none of them is nil this will be nil ?

[].compact! => nil

not sure to understand "If none of them": did you mean "if all them are nil"?
BTW, there will be always at least the first page link (even if it is empty), so that should not happen... however I will try to avoid asumptions. Thanks

if no nil is removed the result will be nil

I still don't get your comment at all :/ (maybe my morning-dumbness... or just dumbness)
How [].compact! relates to the code there? Is that a suggestion or a warning?

grosser · 2019-03-10T14:51:24Z

lib/pagy/frontend.rb

@@ -10,10 +10,23 @@ class Pagy
  # see https://ddnexus.github.io/pagy/api/frontend#i18n
  I18n = eval(Pagy.root.join('locales', 'utils', 'i18n.rb').read) #rubocop:disable Security/Eval

+  module Helpers
+    # This works with all Rack-based frameworks (Sinatra, Padrino, Rails, ...)
+    def pagy_url_for(page, pagy, path_or_url=:path)


Suggested change

def pagy_url_for(page, pagy, path_or_url=:path)

def pagy_url_for(page, pagy, path_or_url: :path)

nicer, but Pagy is ruby 1.9+ compatible since version 2 (I needed to use it in legacy apps with legacy envs)

grosser · 2019-03-10T14:52:46Z

lib/pagy/frontend.rb

+  module Helpers
+    # This works with all Rack-based frameworks (Sinatra, Padrino, Rails, ...)
+    def pagy_url_for(page, pagy, path_or_url=:path)
+      p_vars = pagy.vars; params = request.GET.merge(p_vars[:page_param].to_s => page).merge!(p_vars[:params])


can avoid a merge:

Suggested change

p_vars = pagy.vars; params = request.GET.merge(p_vars[:page_param].to_s => page).merge!(p_vars[:params])

p_vars = pagy.vars; params = request.GET.merge(p_vars[:params])

params[p_vars[:page_param].to_s] = page

we didn't notice that for so long! Thank

grosser · 2019-03-10T14:54:09Z

test/pagy/extras/headers_test.rb

+  end
+
+
+


extra newlines ?

grosser

looks pretty useable and nice that it's opt-in

… code improvements; updated doc and tests

espen · 2021-01-22T12:35:09Z

Didn't see this until now. Works great. Thanks 👏

grosser reviewed Mar 10, 2019

View reviewed changes

grosser approved these changes Mar 10, 2019

View reviewed changes

ddnexus mentioned this pull request Mar 11, 2019

Use "Link" instead of "Links" for header extra #142

Merged

ddnexus and others added 2 commits March 11, 2019 15:39

added headers extra

7d36175

Use "Link" instead of "Links" for header extra (#142)

2254279

ddnexus force-pushed the feature/headers branch from bcab431 to 50773df Compare March 11, 2019 10:02

refactoring: added customizable headers; normalized hash and headers;…

25aadeb

… code improvements; updated doc and tests

ddnexus force-pushed the feature/headers branch from 50773df to 25aadeb Compare March 12, 2019 04:32

ddnexus merged commit 8bd19d6 into dev Mar 12, 2019

ddnexus deleted the feature/headers branch March 12, 2019 04:36

ddnexus added the merged label Mar 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added headers extra #141

added headers extra #141

ddnexus commented Mar 10, 2019 •

edited

Loading

ddnexus commented Mar 10, 2019 •

edited

Loading

grosser Mar 10, 2019

ddnexus Mar 11, 2019

grosser Mar 10, 2019

ddnexus Mar 11, 2019

grosser Mar 10, 2019

ddnexus Mar 11, 2019

grosser Mar 10, 2019

ddnexus Mar 11, 2019

grosser Mar 10, 2019

grosser Mar 10, 2019

ddnexus Mar 11, 2019

grosser Mar 11, 2019

ddnexus Mar 11, 2019

grosser Mar 10, 2019

ddnexus Mar 11, 2019

grosser Mar 11, 2019

ddnexus Mar 11, 2019

grosser Mar 10, 2019

ddnexus Mar 11, 2019

grosser Mar 10, 2019

ddnexus Mar 11, 2019

grosser Mar 10, 2019

ddnexus Mar 11, 2019

grosser left a comment

espen commented Jan 22, 2021

	def pagy_url_for(page, pagy, path_or_url=:path)
	def pagy_url_for(page, pagy, path_or_url: :path)

	p_vars = pagy.vars; params = request.GET.merge(p_vars[:page_param].to_s => page).merge!(p_vars[:params])
	p_vars = pagy.vars; params = request.GET.merge(p_vars[:params])
	params[p_vars[:page_param].to_s] = page

added headers extra #141

added headers extra #141

Conversation

ddnexus commented Mar 10, 2019 • edited Loading

ddnexus commented Mar 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grosser left a comment

Choose a reason for hiding this comment

espen commented Jan 22, 2021

ddnexus commented Mar 10, 2019 •

edited

Loading

ddnexus commented Mar 10, 2019 •

edited

Loading