Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

Remove ks dependency #46

Merged
merged 25 commits into from
Aug 17, 2021
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
language: ruby
rvm:
- 2.4.4
- 2.5.1
- 2.6.2
sudo: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has no effect anymore.

- 2.7.4
- 3.0.2
cache: bundler
env:
global:
Expand Down
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,2 +1,8 @@
## 1.0.0
* Add support for Ruby 2.7 and onwards (including Ruby 3.0)
* Drop support for Ruby 2.6 and lower
* Remove `ks` dependency
* Add `addressable` runtime dependency
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to depend on Addressable? GIven that this is a library. Are there any features in Addressable we are using which are not provided by uri from stdlib?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! URI.encode/URI.escape and URI.decode/URI.unescape have been removed in Ruby 3. For Ruby 3 you have to work with URI.encode_www_form or URI.encode_www_form_component, which have a different API. I tried those, but it was a hassle to get that working. Also, I could only test it on Travis CI and not locally, making matters more difficult. Keeping the time invested already in mind I went for the pragmatic solution proposed in docusign/docusign-esign-ruby-client#45. More context can be found here https://docs.knapsackpro.com/2020/uri-escape-is-obsolete-percent-encoding-your-query-string. Feel free to have a try, as it should be possible, but I can't really spend more time on this than I have.


## 0.8.2
* Change format_parser required version to allow > 0.24
4 changes: 2 additions & 2 deletions DEVELOPMENT.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ path access using the glob whitelist)
class PicFetcher < ImageVise::FetcherFile
def self.fetch_uri_to_tempfile(uri_object)
# Convert an internal "pic://sites/uploads/abcdef.jpg" to a full path URL
partial_path = URI.decode(uri_object.path)
partial_path = Addressable::URI.unencode(uri_object.path)
full_path = File.join(Mappe::ROOT, 'sites', partial_path)
full_path_uri = URI('file://' + URI.encode(full_path))
full_path_uri = URI('file://' + Addressable::URI.encode(full_path))
super(full_path_uri)
end
ImageVise.register_fetcher 'pic', self
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ end

If you want to grab a local file, compose a `file://` URL (mind the endcoding!)

src_url = 'file://' + URI.encode(File.expand_path(my_pic))
src_url = 'file://' + Addressable::URI.encode(File.expand_path(my_pic))

Note that you need to permit certain glob patterns as sources before this will work, see below.

Expand Down
6 changes: 4 additions & 2 deletions image_vise.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -22,25 +22,27 @@ Gem::Specification.new do |spec|
raise "RubyGems 2.0 or newer is required to protect against public gem pushes."
end

spec.required_ruby_version = ">= 2.7"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason we need to require 2.7+? Keyword init for structs is available since 2.5 and even a few internal WT apps are still using 2.6+

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabioperrella suggested to only support the stable releases. Ruby 2.5 is not maintained anymore and the security maintenance for 2.6 will also end soon (https://www.ruby-lang.org/en/downloads/). I'm happy to lower this, but I'll leave that decision up to you and Fabio.

Copy link
Contributor

Choose a reason for hiding this comment

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

@julik is there a reason to keep compatibility with EOL Ruby versions?

@kodnin I think we need to support >= 2.6 because the end of life will happen only in 7 months -> https://endoflife.date/ruby, wdyt?

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'm trying to make it work for Ruby 2.6.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works™.


spec.files = `git ls-files -z`.split("\x0")
spec.bindir = "exe"
spec.executables = spec.files.grep(%r{^exe/}) { |f| File.basename(f) }
spec.require_paths = ["lib"]

spec.add_dependency 'patron', '~> 0.9'
spec.add_dependency 'rmagick', '~> 3'
spec.add_dependency 'ks'
spec.add_dependency 'rack', '>= 1', '< 3'
spec.add_dependency 'format_parser', '~> 0.25'
spec.add_dependency 'measurometer', '~> 1'
spec.add_dependency "addressable"
kodnin marked this conversation as resolved.
Show resolved Hide resolved

spec.add_development_dependency 'magic_bytes', '~> 1'
spec.add_development_dependency "bundler"
spec.add_development_dependency "rake", "~> 12.2"
spec.add_development_dependency "rack-test"
spec.add_development_dependency "rspec", "~> 3"
spec.add_development_dependency "addressable"
spec.add_development_dependency "strenv"
spec.add_development_dependency "simplecov"
spec.add_development_dependency "pry"
spec.add_development_dependency "webrick"
end
1 change: 0 additions & 1 deletion lib/image_vise.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
require 'ks'
require 'json'
require 'patron'
require 'rmagick'
Expand Down
2 changes: 1 addition & 1 deletion lib/image_vise/fetchers/fetcher_file.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def self.fetch_uri_to_tempfile(uri)
end

def self.uri_to_path(uri)
File.expand_path(URI.decode(uri.path))
File.expand_path(Addressable::URI.unencode(uri.path))
end

def self.verify_filesystem_access!(path_on_filesystem)
Expand Down
2 changes: 1 addition & 1 deletion lib/image_vise/image_request.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require 'openssl'

class ImageVise::ImageRequest < Ks.strict(:src_url, :pipeline)
class ImageVise::ImageRequest < Struct.new(:src_url, :pipeline, keyword_init: true)
class InvalidRequest < ArgumentError; end
class SignatureError < InvalidRequest; end
class URLError < InvalidRequest; end
Expand Down
2 changes: 1 addition & 1 deletion lib/image_vise/operators/background_fill.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
# Can handle most 'word' colors and hex color codes but not RGB values.
#
# The corresponding Pipeline method is `background_fill`.
class ImageVise::BackgroundFill < Ks.strict(:color)
class ImageVise::BackgroundFill < Struct.new(:color, keyword_init: true)
def initialize(*)
super
self.color = color.to_s
Expand Down
2 changes: 1 addition & 1 deletion lib/image_vise/operators/crop.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
# of ImageMagick gravity parameters (see GRAVITY_PARAMS)
#
# The corresponding Pipeline method is `crop`.
class ImageVise::Crop < Ks.strict(:width, :height, :gravity)
class ImageVise::Crop < Struct.new(:width, :height, :gravity, keyword_init: true)
GRAVITY_PARAMS = {
'nw' => Magick::NorthWestGravity,
'n' => Magick::NorthGravity,
Expand Down
2 changes: 1 addition & 1 deletion lib/image_vise/operators/expire_after.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# Overrides the cache lifetime set in the output headers of the RenderEngine.
# Can be used to permit the requester to set the caching lifetime, instead
# of it being a configuration variable in the service performing the rendering
class ImageVise::ExpireAfter < Ks.strict(:seconds)
class ImageVise::ExpireAfter < Struct.new(:seconds, keyword_init: true)
def initialize(seconds:)
unless seconds.is_a?(Integer) && seconds > 0
raise ArgumentError, "the :seconds parameter must be an Integer and must be above 0, but was %s" % seconds.inspect
Expand Down
2 changes: 1 addition & 1 deletion lib/image_vise/operators/fit_crop.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
# gravity parameter defines the crop gravity (on corners, sides, or in the middle).
#
# The corresponding Pipeline method is `fit_crop`.
class ImageVise::FitCrop < Ks.strict(:width, :height, :gravity)
class ImageVise::FitCrop < Struct.new(:width, :height, :gravity, keyword_init: true)
GRAVITY_PARAMS = {
'nw' => Magick::NorthWestGravity,
'n' => Magick::NorthGravity,
Expand Down
2 changes: 1 addition & 1 deletion lib/image_vise/operators/force_jpg_out.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# Forces the output format to be JPEG and specifies the quality factor to use when saving
#
# The corresponding Pipeline method is `force_jpg_out`.
class ImageVise::ForceJPGOut < Ks.strict(:quality)
class ImageVise::ForceJPGOut < Struct.new(:quality, keyword_init: true)
def initialize(quality:)
unless (0..100).cover?(quality)
raise ArgumentError, "the :quality setting must be within 0..100, but was %d" % quality
Expand Down
2 changes: 1 addition & 1 deletion lib/image_vise/operators/geom.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# Applies a transformation using an ImageMagick geometry string
#
# The corresponding Pipeline method is `geom`.
class ImageVise::Geom < Ks.strict(:geometry_string)
class ImageVise::Geom < Struct.new(:geometry_string, keyword_init: true)
def initialize(*)
super
self.geometry_string = geometry_string.to_s
Expand Down
2 changes: 1 addition & 1 deletion lib/image_vise/operators/sharpen.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# Applies a sharpening filter to the image.
#
# The corresponding Pipeline method is `sharpen`.
class ImageVise::Sharpen < Ks.strict(:radius, :sigma)
class ImageVise::Sharpen < Struct.new(:radius, :sigma, keyword_init: true)
def initialize(*)
super
self.radius = radius.to_f
Expand Down
6 changes: 3 additions & 3 deletions lib/image_vise/pipeline.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@ def empty?
@ops.empty?
end

def method_missing(method_name, *a, &blk)
def method_missing(method_name, args = {}, &blk)
operator_builder = ImageVise.operator_from(method_name)
self << operator_builder.new(*a)
self << operator_builder.new(**args)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the 2.7 keyword argument treatment the reason we need to require 2.7? If so it is unpleasant but there likely is a solution we could try to stay backwards-compatible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the keyword argument treatment in Ruby 3 that is the problem (https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/). Ruby 2.7 builds fine without this change, Ruby 3 doesn't. Do you have a suggestion to make it backwards-compatible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wondering, isn't it backwards compatible already?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like it is, I think

As I suggested in another discussion, if we add support to Ruby 2.6 as well, Travis will tell us if this works 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

What about something like this?

def method_missing(method_name, args = {}, &blk)
  operator_builder = ImageVise.operator_from(method_name)
  # TODO: When we remove support for Ruby 2.6, we can remove this condition
  if args == {}
    self << operator_builder.new
  else
    self << operator_builder.new(**args)
  end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I did this 5caf08c. I can try your solution as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There you go, fd583fb.

Copy link
Contributor

Choose a reason for hiding this comment

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

it is good for me!

end

def respond_to_missing?(method_name, *a)
ImageVise.defined_operators.include?(method_name.to_s)
end
Expand Down
2 changes: 1 addition & 1 deletion lib/image_vise/version.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
class ImageVise
VERSION = '0.8.2'
VERSION = '1.0.0'
end
2 changes: 1 addition & 1 deletion lib/image_vise/writers/jpeg_writer.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
class ImageVise::JPGWriter < Ks.strict(:quality)
class ImageVise::JPGWriter < Struct.new(:quality, keyword_init: true)
JPG_EXT = 'jpg'

def write_image!(magick_image, _, render_to_path)
Expand Down
12 changes: 6 additions & 6 deletions spec/image_vise/fetcher_file_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@
it 'is registered as a fetcher for file://' do
expect(ImageVise.fetcher_for('file')).to eq(ImageVise::FetcherFile)
end

it 'returns a Tempfile containing this test suite' do
path = File.expand_path(__FILE__)
ruby_files_in_this_directory = __dir__ + '/*.rb'
ImageVise.allow_filesystem_source! ruby_files_in_this_directory
uri = URI('file://' + URI.encode(path))

uri = URI('file://' + Addressable::URI.encode(path))
fetched = ImageVise::FetcherFile.fetch_uri_to_tempfile(uri)

expect(fetched).to be_kind_of(Tempfile)
Expand All @@ -27,7 +27,7 @@

ImageVise.deny_filesystem_sources!

uri = URI('file://' + URI.encode(path))
uri = URI('file://' + Addressable::URI.encode(path))
expect {
ImageVise::FetcherFile.fetch_uri_to_tempfile(uri)
}.to raise_error(ImageVise::FetcherFile::AccessError)
Expand All @@ -40,7 +40,7 @@
ImageVise.deny_filesystem_sources!
ImageVise.allow_filesystem_source! text_files_in_this_directory

uri = URI('file://' + URI.encode(path))
uri = URI('file://' + Addressable::URI.encode(path))
expect {
ImageVise::FetcherFile.fetch_uri_to_tempfile(uri)
}.to raise_error(ImageVise::FetcherFile::AccessError)
Expand All @@ -51,7 +51,7 @@
ruby_files_in_this_directory = __dir__ + '/*.rb'
ImageVise.allow_filesystem_source! ruby_files_in_this_directory

uri = URI('file://' + URI.encode(path))
uri = URI('file://' + Addressable::URI.encode(path))
expect(ImageVise::FetcherFile).to receive(:maximum_source_file_size_bytes).and_return(1)

expect {
Expand Down
4 changes: 2 additions & 2 deletions spec/image_vise/render_engine_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ def raise_exceptions?
it 'URI-decodes the path in a file:// URL for a file with a Unicode path' do
utf8_file_path = File.dirname(test_image_path) + '/картинка.jpg'
FileUtils.cp_r(test_image_path, utf8_file_path)
uri = 'file://' + URI.encode(utf8_file_path)
uri = 'file://' + Addressable::URI.encode(utf8_file_path)

ImageVise.allow_filesystem_source!(File.dirname(test_image_path) + '/*.*')
ImageVise.add_secret_key!('l33tness')
Expand All @@ -264,7 +264,7 @@ def raise_exceptions?
end

it 'forbids a request with an extra GET param' do
uri = 'file://' + URI.encode(test_image_path)
uri = 'file://' + Addressable::URI.encode(test_image_path)

p = ImageVise::Pipeline.new.fit_crop(width: 10, height: 10, gravity: 'c')
image_request = ImageVise::ImageRequest.new(src_url: uri.to_s, pipeline: p)
Expand Down