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

Avoid using the last argument as keyword parameter #248

Closed
wants to merge 1 commit into from

Conversation

LinusU
Copy link

@LinusU LinusU commented Apr 14, 2021

This fixes the following warning when running the code with Ruby 2.7:

gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated

Fastlane is still using the 1.x version of this library, and this warning is printed as soon as it's launched. It think it would be great if this could be released as a patch release (e.g. 1.7.11) so that the warning is no longer printed:

Screenshot 2021-04-14 at 14 14 24

This fixes the following warning when running the code with Ruby 2.7:

```text
gems/highline-1.7.10/lib/highline.rb:624: warning: Using the last argument as keyword parameters is deprecated
```
@ainame
Copy link

ainame commented Apr 23, 2021

@LinusU 👋 from fastlane core team, who is in charge of Ruby 3.0 support. Thank you for looking into this issue in fastlane! I've been recognising this and will be working on the migration soon.

That said, it doesn't prevent the gem owner from releasing a patched version though. I'll leave this PR.

@abinoam
Copy link
Collaborator

abinoam commented Apr 27, 2021

Hi @LinusU,

Thank you very much for your time spent on this.

Do you mind if I postpone the decision to merge and release it?

I've had no plan to release from 1-7-stable unless it would be a security patch, which is not the case.

I'm more prone to help people with this kind of problem to migrate to HighLine 2.

So, if @ainame has any problem migrating to HighLine 2 I would be available to help him (and any other open source maintainer) to do so.

Thank you again.

@LinusU
Copy link
Author

LinusU commented Apr 27, 2021

@abinoam sure thing, no stress 👍

It seems like Fastlane is moving to 2.x sooner rather than later so this would be resolved then

@ainame
Copy link

ainame commented Apr 29, 2021

@abinoam Thank you very much for your offer🙇

I have a question in migration 1.7.2 -> 2.0.3. I've checked the changelog file but it was a bit unclear to me that what is actually breaking changes. From what I see on the changelog, this is the only breaking API change between 1.7.2 and 2.0.0, isn't this?

PR #214 - Remove $terminal (global variable) (@abinoam)

fastlane support Ruby from 2.4 or newer so the changes in support Ruby version/environment wouldn't matter for us.

(fastlane also depends on commander gem that uses highline and they just fixed above commander-rb/commander#73)

@abinoam
Copy link
Collaborator

abinoam commented Apr 29, 2021

Hi @ainame,

You're right. We have made our best effort to maintain near 100% percent compatibility as HighLine is a really old piece of software with a lot of other libs depending on it.

We have removed support to global variables as they are inherently evil most of the time.
So the recommended way of use is to instantiate HighLine, save it somewhere and use from there without polluting the global namespace.

We have created 'highline/import' so if you consciously want to inject the HighLine methods into Kernel so they are globally available. The result would be probably 100% the same as using the old version of HighLine.

require 'highline'

# Basic usage

cli = HighLine.new
answer = cli.ask "What do you think?"
puts "You have answered: #{answer}"
require 'highline/import'

say "Now you can use #say directly"

Most people reported that the transition was smoothly no matter which way you choose.

Some other changes are related to "internal" implementation. So if the users' code rely on that level of detail it may fail when upgrading.

@ainame
Copy link

ainame commented Apr 29, 2021

You're right. We have made our best effort to maintain near 100% percent compatibility as HighLine is a really old piece of software with a lot of other libs depending on it.

That sounds amazing! I really respect and appricate all of your works on that effort. As far as I checked fastlane doesn't use $terminal but used require 'highline/import' so I will look into a way to get rid of global references🙂

I will create another ticket if I get any issues in our migration🙇 Thank you very much.

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.

None yet

3 participants