-
Notifications
You must be signed in to change notification settings - Fork 414
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
URIs are always joined with forward slash #11
Conversation
lib/pagy/frontend.rb
Outdated
@@ -34,7 +34,7 @@ def pagy_info(pagy) | |||
|
|||
# this works with all Rack-based frameworks (Sinatra, Padrino, Rails, ...) | |||
def pagy_url_for(n) | |||
url = File.join(request.script_name.to_s, request.path_info) | |||
url = "#{request.script_name}/#{request.path_info}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this is not even needed. According to Rack specification:
The
SCRIPT_NAME
, if non-empty, must start with/
ThePATH_INFO
, if non-empty, must start with/
One ofSCRIPT_NAME
orPATH_INFO
must be set.PATH_INFO
should be/
ifSCRIPT_NAME
is empty.SCRIPT_NAME
never should be/
, but instead be empty.
And in fact Rack
provides a handy #path
method for exactly this purpose:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So why don't we use just url = request.path
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be my recommendation as well. I’ll update the PR, perhaps add a few tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
`File.join` might use backslash on e.g. Windows - URL paths are not File paths. According to Rack specification: - The `SCRIPT_NAME`, if non-empty, must start with `/` - The `PATH_INFO`, if non-empty, must start with `/` - One of `SCRIPT_NAME` or `PATH_INFO` must be set. `PATH_INFO` should be `/` if `SCRIPT_NAME` is empty. `SCRIPT_NAME` never should be `/`, but instead be empty. And in fact Rack provides this handy `#path` method for exactly this purpose: https://github.com/rack/rack/blob/42e48013dd1b6dbd/lib/rack/request.rb#L406-L408
c0bab60
to
8543cd1
Compare
@@ -23,6 +23,7 @@ Gem::Specification.new do |s| | |||
s.add_development_dependency 'bundler', '~> 1.16' | |||
s.add_development_dependency 'rake', '~> 10.0' | |||
s.add_development_dependency 'minitest', '~> 5.0' | |||
s.add_development_dependency 'rack' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t know if rack
should in fact be considered a runtime dependency? You cannot use Pagy::Frontend
without either having Rack
loaded, or overriding Pagy::Frontend#pagy_url_for
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot use Pagy::Frontend without either having Rack loaded, or overriding Pagy::Frontend#pagy_url_for
True, but...
The pagy_url_for
uses rack just to give a default usable on most apps out of the box. Adding an explicit dependency would cause the installation of rack even if you never intended to use rack in your app and you are fine with just overriding the pagy_url_for
.
On the other hand not adding it would cause the method to fail until the user will provide his/her own way to create the url, with no feedback added.
The best (puristic) approach would probably be conditionally defining the method in presence of rack, and defining the method raising a NotImplementedError
exception if Rack is not present.
Something similar to how the pagy_t
is implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that sounds like a valid strategy. Except, just use NoMethodError
instead of NotImplementedError
: http://chrisstump.online/2016/03/23/stop-abusing-notimplementederror/ (as in: just leave the method not implemented, or raise it with a message about using Rack)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I improperly used NotImplementedError
till now :/.
If we leave it not implemented, there will be no feedback, if we implement it and raise the NoMethodError
with a message, that would work, however, respond_to?
would return true
, but I can live with that :).
I think we should raise the NoMethodError
with a clear self-explanatory feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want it implemented in this PR, or a new one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with both options. Whatever is better for you. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrapped the method in an if
block.
But then I thought of another thing: the method does not only require Rack, in order to call #GET
and #path
on the Request. It also requires a #request
method in the class where the module is included…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... there is also the params
method used by the Pagy::Backend#pagy_get_variables
...
I think we should just drop this attempt to be so smart, over-complicating our life, and try to be just practical :). I think that that "problems" could be addressed in a "Requirements" or similar section where we specifically say what is needed to make it work.
For now, I would focus on fixing the bug and eventually adding the tests (we could add rack as a development dependence), because the bug could affect users right now.
Then, if we will find some better way than the "Requirement" section, we could implement it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I have removed the last commit, so this PR is back at just using request.path
and adding a few tests where Rack
needs to be a development dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
8543cd1
to
fa52fa8
Compare
@ddnexus Ready for review. |
1765527
to
92260b5
Compare
c04859d
to
fa52fa8
Compare
File.join
might use backslash on e.g. Windows - URL paths are not File paths.According to Rack specification:
SCRIPT_NAME
, if non-empty, must start with/
PATH_INFO
, if non-empty, must start with/
SCRIPT_NAME
orPATH_INFO
must be set.PATH_INFO
should be/
ifSCRIPT_NAME
is empty.SCRIPT_NAME
never should be/
, but instead beempty.
And in fact Rack provides this handy
#path
method for exactly this purpose: https://github.com/rack/rack/blob/42e48013dd1b6dbd/lib/rack/request.rb#L406-L408