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

URIs are always joined with forward slash #11

Merged
merged 2 commits into from
May 27, 2018

Conversation

bquorning
Copy link
Contributor

@bquorning bquorning commented May 25, 2018

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

@@ -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}"
Copy link
Contributor Author

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 /
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 a handy #path method for exactly this purpose:

https://github.com/rack/rack/blob/42e48013dd1b6dbda990dfa3851856c199b0b1f9/lib/rack/request.rb#L406-L408

Copy link
Owner

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?

Copy link
Contributor Author

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.

Copy link
Owner

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
@bquorning bquorning force-pushed the uri-is-not-a-file branch from c0bab60 to 8543cd1 Compare May 26, 2018 12:10
@bquorning bquorning changed the base branch from master to dev May 26, 2018 12:11
@@ -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'
Copy link
Contributor Author

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.

Copy link
Owner

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.

Copy link
Contributor Author

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)

Copy link
Owner

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.

Copy link
Contributor Author

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?

Copy link
Owner

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.

Copy link
Contributor Author

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…

Copy link
Owner

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.

Copy link
Contributor Author

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.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks

@bquorning bquorning force-pushed the uri-is-not-a-file branch from 8543cd1 to fa52fa8 Compare May 26, 2018 12:16
@bquorning
Copy link
Contributor Author

@ddnexus Ready for review.

@ddnexus ddnexus added the bug label May 27, 2018
@ddnexus ddnexus force-pushed the dev branch 2 times, most recently from 1765527 to 92260b5 Compare May 27, 2018 05:03
@bquorning bquorning force-pushed the uri-is-not-a-file branch from c04859d to fa52fa8 Compare May 27, 2018 12:36
@ddnexus ddnexus merged commit 144c997 into ddnexus:dev May 27, 2018
@bquorning bquorning deleted the uri-is-not-a-file branch May 28, 2018 04:34
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