Remove ks dependency #46

kodnin · 2021-07-29T10:58:53Z

Remove ks gem as it breaks Ruby 3 compatibility. image_vise is (still) used in https://github.com/WeTransfer/spaceship.

kodnin · 2021-07-30T07:42:51Z

Reminder: we need to look into what Ruby versions are supported, because not all have this Struct option yet. That was the whole point of the ks gem.

fabioperrella · 2021-07-30T08:46:21Z

Well, we can bump the major and remove support of old Rubies.

As a suggestion, you can change this project to use Github Actions as I proposed here WeTransfer/sqewer#88

kodnin · 2021-08-04T13:18:00Z

Well, we can bump the major and remove support of old Rubies.

As a suggestion, you can change this project to use Github Actions as I proposed here WeTransfer/sqewer#88

I'm currently awaiting a response on https://github.com/WeTransfer/image_vise_standalone/issues/300#issuecomment-892649130.

kodnin · 2021-08-10T13:00:49Z

.travis.yml

- 2.4.4
- 2.5.1
- 2.6.2
-sudo: false


This has no effect anymore.

julik

The ks removal is perfect 😍 but there are a couple of other points we should probably look into

julik · 2021-08-10T22:04:49Z

CHANGELOG.md

+* 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


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?

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.

julik · 2021-08-10T22:05:46Z

image_vise.gemspec

@@ -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"


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+

@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.

@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?

I'm trying to make it work for Ruby 2.6.

It works™.

julik · 2021-08-10T22:06:59Z

lib/image_vise/pipeline.rb

    operator_builder = ImageVise.operator_from(method_name)
-    self << operator_builder.new(*a)
+    self << operator_builder.new(**args)


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

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?

Just wondering, isn't it backwards compatible already?

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 😄

It is not: https://app.travis-ci.com/github/WeTransfer/image_vise/jobs/530652721.

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

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

There you go, fd583fb.

it is good for me!

image_vise.gemspec

Co-authored-by: Fabio Perrella <[email protected]>

lib/image_vise/pipeline.rb

fabioperrella

nice job!

kodnin · 2021-08-11T14:40:31Z

nice job!

Cool, then it's up to @julik to decide. Who's going to do a gem release?

no need to remove backwards-compat just yet

We can test only on "oldest" and "latest", "in-between" might not be required

we will deal with it later, for now it is good enough simply not to require it for using ImageVise

fabioperrella · 2021-08-17T08:43:42Z

@julik I would like to understand why we still need to support Ruby 2.5 which is already an EOL version

julik · 2021-08-17T10:45:45Z

@fabioperrella Sure! I think removing support for something on a library that people use is a balance between effort to keep supporting it and the use of the library by others. Even though a version of Ruby is EOL, as long as we can cheaply test against it (and it doesn't require too many workarounds) when we are providing a library we should keep support for older versions as long as is practical. I use image_vise in a few of my own apps, and I don' remember which ones of them are still on 2.5 - just an example. So I try to look at it from the perspective of a user: if we drop support for a runtime version, we cause them inconvenience. Some would not be able to update image_vise due to this requirement because their app might be using something that can't run on 2.6+.

So: the line of thought that I have here is that if it is not a large effort to maintain compatibility with older versions of Ruby we, as library authors, absolutely should maintain it.

fabioperrella · 2021-08-17T13:14:38Z

ok got it, thank you for the explanation!

kodnin added 2 commits July 29, 2021 12:50

Remove ks dependency

a627baf

Also test on Ruby 3.0.2

29a25ff

kodnin marked this pull request as ready for review July 29, 2021 11:48

kodnin added 4 commits August 9, 2021 10:28

Only support newer versions of Ruby

75f81f2

Update CHANGELOG

7c34b6b

Add language to Travis config

a8914e5

Drop sudo from Travis config

47c21f4

kodnin force-pushed the remove-ks-dependency branch from 0d0e55b to 47c21f4 Compare August 10, 2021 12:49

kodnin commented Aug 10, 2021

View reviewed changes

.travis.yml

- 2.4.4

- 2.5.1

- 2.6.2

sudo: false

Copy link

Contributor Author

kodnin Aug 10, 2021

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.

kodnin added 2 commits August 10, 2021 15:08

Use URI.encode_www_form_component instead of URI.encode

930ee2e

Add webrick as a development dependency

3563018

kodnin force-pushed the remove-ks-dependency branch from ae26e9a to 3563018 Compare August 10, 2021 13:08

kodnin added 2 commits August 10, 2021 15:39

Use Addressable::URI.encode

630f1bc

Use Addressable::URI.unencode

2f96c85

kodnin force-pushed the remove-ks-dependency branch 2 times, most recently from a786851 to 0275e74 Compare August 10, 2021 15:08

Pass arguments as a hash

ed8bd17

kodnin force-pushed the remove-ks-dependency branch from 0275e74 to ed8bd17 Compare August 10, 2021 15:13

Update CHANGELOG with addressable dependency

29dac98

julik self-requested a review August 10, 2021 15:26

julik suggested changes Aug 10, 2021

View reviewed changes

fabioperrella reviewed Aug 11, 2021

View reviewed changes

image_vise.gemspec Outdated Show resolved Hide resolved

kodnin and others added 3 commits August 11, 2021 12:20

Add Ruby 2.6 support

01f71ec

Update image_vise.gemspec

14cb0af

Co-authored-by: Fabio Perrella <[email protected]>

Handle keyword arguments for Ruby 2.6

5caf08c

kodnin force-pushed the remove-ks-dependency branch from 97c1ccf to 5caf08c Compare August 11, 2021 11:31

kodnin commented Aug 11, 2021

View reviewed changes

lib/image_vise/pipeline.rb Outdated Show resolved Hide resolved

kodnin force-pushed the remove-ks-dependency branch 2 times, most recently from 053b755 to a20fc6d Compare August 11, 2021 11:47

Improve Ruby 2.6 compatibility hack

fd583fb

kodnin force-pushed the remove-ks-dependency branch from a20fc6d to fd583fb Compare August 11, 2021 11:56

fabioperrella approved these changes Aug 11, 2021

View reviewed changes

julik added 9 commits August 14, 2021 00:19

Reduce required Ruby to 2.5

4b6a238

no need to remove backwards-compat just yet

Remove Bundler lock

0332274

Increase Ruby version spread for tests

61a4434

We can test only on "oldest" and "latest", "in-between" might not be required

Remove use of Addressable in specs

a3fa2f5

Make addressable a development dependency

9e82a48

we will deal with it later, for now it is good enough simply not to require it for using ImageVise

Remove Addressable from DEVELOPMENT too

dc2724f

Remove the rest of Addressable

933acea

Remove addressable require

5e7634a

Update CHANGELOG

5fbc8ac

julik merged commit 83829e7 into master Aug 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove ks dependency #46

Remove ks dependency #46

kodnin commented Jul 29, 2021

kodnin commented Jul 30, 2021

fabioperrella commented Jul 30, 2021

kodnin commented Aug 4, 2021

kodnin Aug 10, 2021

julik left a comment

julik Aug 10, 2021

kodnin Aug 11, 2021

julik Aug 10, 2021

kodnin Aug 11, 2021

fabioperrella Aug 11, 2021

kodnin Aug 11, 2021

kodnin Aug 11, 2021

julik Aug 10, 2021

kodnin Aug 11, 2021

kodnin Aug 11, 2021

fabioperrella Aug 11, 2021

kodnin Aug 11, 2021

fabioperrella Aug 11, 2021

kodnin Aug 11, 2021

kodnin Aug 11, 2021

fabioperrella Aug 11, 2021

fabioperrella left a comment

kodnin commented Aug 11, 2021

fabioperrella commented Aug 17, 2021

julik commented Aug 17, 2021

fabioperrella commented Aug 17, 2021

Remove ks dependency #46

Remove ks dependency #46

Conversation

kodnin commented Jul 29, 2021

kodnin commented Jul 30, 2021

fabioperrella commented Jul 30, 2021

kodnin commented Aug 4, 2021

Choose a reason for hiding this comment

julik left a 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

fabioperrella left a comment

Choose a reason for hiding this comment

kodnin commented Aug 11, 2021

fabioperrella commented Aug 17, 2021

julik commented Aug 17, 2021

fabioperrella commented Aug 17, 2021