Upgrade to Ruby 3.1 #32
Conversation
| @@ -78,7 +78,7 @@ def set_downtime_host(host, author, comment, start_time, end_time, all_services: | |||
| private | |||
|
|
|||
| def uri_encode_filter(filter) | |||
| URI.encode(filter) | |||
| CGI.escape(filter) | |||
There was a problem hiding this comment.
CGI.escape is quite a different API from URI.encode. I opened #30 and would prefer to investigate that path instead.
There was a problem hiding this comment.
I changed the required_ruby_version from '>= 2.5', '< 3' to '>= 2.7', '< 4' and we are not getting any URI.escape error any more. So I move back to the previous change. All checks green though.
There was a problem hiding this comment.
You can see that the URL that's stubbed is changed, and it introduces a problem.
In the old version the parameters are separated by & but in the new version that's now escaped and represented as %26%26. This means only two parameters (filter and type) are sent, instead of all 4 (filter, author, comment, type).
@ekohl Done! But wouldn't that be good to separate both the changes in diff PRs- like one is all about ruby 3 upgrdae and other one is about the functionality change. This way anyone can validate those changes and able to easily track it too. So if you can prefer to merge #30 first before switching to this PR? |
|
Merged #30 |
This avoids needing to URL encode the parameters and simplifies the code. This removes the need for URI.encode, which is removed in Ruby 3.
|
This appears to still include a second commit that appears to be a modification of a previously merged change. I'd recommend dropping that commit and re-pushing. |
|
I rebased & merged because I thought it would drop the commit, but looks like there was also a change in it so there is now 8b924eb. |
Adding Ruby v3 supports.
Requires: