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

Problem with remote files #52

Closed
wants to merge 1 commit into from
Closed

Problem with remote files #52

wants to merge 1 commit into from

Conversation

viniciusalonso
Copy link

Now is possible use a remote file, with the temporary file and your usage is normal equal of the Phashion::Image.

@mperham
Copy link
Collaborator

mperham commented Nov 27, 2014

Why build this functionality into Phashion directly when you can do this?

require 'open-uri'

open("http://...") do |f|
  i = Phashion::Image.new(f)
  ...
end

@viniciusalonso
Copy link
Author

Because, i found that would better organized in other class.
And if in future it takes add more behaviours the remote files, not will be necessary refactor code

@westonplatter
Copy link
Owner

@viniciusalonso I'm thinking about @mperham's question - why add this to pHashion?

IMHO adding HTTP functionality to pHashion enlarges scope and adds complexity to the pHashion API. I think fetching remote images would fit really well as a separate gem that uses pHashion as a dependency.

I think a seprate gem would more provide the flexibility and freedom to address issues like:

  • handling HTTP errors/statuses
  • delegating HTTP interactions to configurable HTTP clients like Faraday, RestClient, Typhoeus
  • "caching" images locally between image comparisons

@viniciusalonso
Copy link
Author

Ok @westonplatter ... What you think about use the solution proposal by @mperham , for solution this problem of simple form ?
And case, in future if need a extend the functionally, create a new gem.

I believe would be more feasible at the moment

@fellipebrito
Copy link

👍
I like what @viniciusalonso did. It fits well in some of the ways I use Phashion right now.

I understand the point that it can add complexity to Phashion, however one of the main targets of the gem is to detect duplicate and near-duplicate multimedia files. Thus, it does not matter if the image is in localhost or remote.

Great job @viniciusalonso, and all the folks who created this gem. It is pretty useful.

Thanks!

@viniciusalonso
Copy link
Author

Now i refactor code, and it is more simple and not increases the scope of gem

@westonplatter
Copy link
Owner

@fellipebrito - Thanks for the additional feedback.

@viniciusalonso - Thanks for the code changes.

What I'm thinking is I'll code up a quick Proof of Concept showing how I'd think we could wrap the remote file functionality in another gem. I'd love to get your input on the idea. I'm still doing Thanksgiving weekend with the family so I should be able to get to it next week.

@fellipebrito
Copy link

@westonplatter Enjoy the thanksgiving time with your family man. Thanks for the GEM ;)

Let me know if you wanna pair a little bit and talk about the other gem idea, I'd be glad to help you to create something else for the community.

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants