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

Dont echo assignment expressions #12

Merged
merged 1 commit into from
Aug 4, 2019

Conversation

onlynone
Copy link
Contributor

@onlynone onlynone commented Mar 21, 2019

Same idea as #11 but it uses RubyVM::AbstractSyntaxTree ripper rather than the parser gem. It would be nice to have a solution that's back-portable to ruby versions prior to 2.6 though. I'm open to any ideas.

@ioquatix
Copy link
Member

I think this is a really awesome idea.

@onlynone
Copy link
Contributor Author

@hsbt Is this a better implementation than #11 ? I guessed on what the new configuration setting should be called (echo_on_assignment which defaults to false), but I'd still like to hear any feedback on it.

I also wrote it so that the echoing isn't suppressed when running ruby < 2.6.0 because RubyVM::AbstractSyntaxTree isn't available until then. If there's another library or technique I could use to figure out the expression type that would work for earlier ruby versions, I'd love to know.

Could we do something like not require the parser gem, but if it's available and RubyVM::AbstractSyntaxTree is not, then try and use it?

@eregon
Copy link
Member

eregon commented Mar 22, 2019

Anything under RubyVM is not portable and CRuby-specific.
Would using Ripper works? That's a fairly portable standard library.

@onlynone
Copy link
Contributor Author

@eregon Thanks for the tip about Ripper. Just updated the patch to use Ripper instead and removed the restriction for 2.6.0.

@hsbt
Copy link
Member

hsbt commented Mar 28, 2019

@onlynone Can you add test for this change?

lib/irb.rb Outdated Show resolved Hide resolved
lib/irb.rb Outdated Show resolved Hide resolved
@onlynone
Copy link
Contributor Author

onlynone commented Mar 28, 2019

@hsbt

Can you add test for this change?

Tests added.

@stomar
Copy link
Contributor

stomar commented Mar 29, 2019

For :opassign echoing the assigned value might be useful.

# `option` might have been set earlier

>> option ||= "default"
=> "value"

Opinions?

@onlynone
Copy link
Contributor Author

onlynone commented Apr 1, 2019

@stomar it sounds like you'd want to set context.echo_on_assignment = true:

>> context.echo_on_assignment = true
=> true
>> option ||= "default"
=> "default"

or puts option ||= "default":

>> puts option ||= "default"
default
=> nil

or just type option at the prompt again:

>> option ||= "default"
>> option
=> "default"

@onlynone
Copy link
Contributor Author

onlynone commented Apr 4, 2019

Would it be better if the default for echo_on_assignment was true so that no behavior changes unless one explicitly sets IRB.conf[:ECHO_ON_ASSIGNMENT] = false or uses --noecho-on-assignment ? I certainly prefer to suppress the echo of assignment expressions, but I can understand if we don't want behavior to change by default.

@onlynone
Copy link
Contributor Author

@nobu just checking in to see if there's anything else that can be done.

@nobu
Copy link
Member

nobu commented Apr 26, 2019

Seems @aycabta working on the overhaul of irb now, stay tuned plz.

@onlynone
Copy link
Contributor Author

Didn't @k0kubun just commit a bunch of stuff? Also, it looks like a lot of that came from ruby/ruby. Should I be making a PR there first?

@k0kubun
Copy link
Member

k0kubun commented Apr 27, 2019

Making changes to ruby/ruby first was my mistake. You're filing PR to the perfect place.

I'm just a random contributor. Ask @aycabta about what's going on around irb, or @hsbt for how we should make changes for defaut gems (I commented it in the above line though).

@onlynone
Copy link
Contributor Author

@nobu It's almost been a month since you asked me to stay tuned, is there any update? @aycabta how's the overhaul going?

@onlynone onlynone force-pushed the dont_echo_assignment_expressions branch 3 times, most recently from eba26ac to 8ce9ce7 Compare June 17, 2019 18:10
@onlynone
Copy link
Contributor Author

@nobu @hsbt are @aycabta's changes all set now? I've rebased on top of master and fixed conflicts. The only failing test seems unrelated to my changes.

@onlynone onlynone force-pushed the dont_echo_assignment_expressions branch from 8ce9ce7 to 0a3a0f5 Compare June 17, 2019 19:40
@onlynone
Copy link
Contributor Author

Failing test was actually related... fixed and squashed all commits.

@onlynone
Copy link
Contributor Author

@nobu @hsbt and @aycabta, just checking in again. It seem like commits have been going in to master again. Any reason to keep holding off?

lib/irb.rb Show resolved Hide resolved
@aycabta aycabta dismissed their stale review August 3, 2019 20:39

OK, I understand the Ripper's maintenance condition.

@aycabta aycabta merged commit 8654953 into ruby:master Aug 4, 2019
@JorgenIvarsson
Copy link

JorgenIvarsson commented Jun 13, 2020

Hi! As a newbie on ruby on rails I feel that something has gone seriously wrong here. I have just searched the Internet, asked in forum for 24 hours, before finding the reason that irb does not echo the object I am creating, while following guides on learning Rails. After testing if this new behavior was caused by the switch from bash to zsh (on Mac), my rails configuration and so on, I finally got an answer on the Rails forum that from version 2.7, Ruby no longer echoes the return value when creating and assigning a new object. I also learned from another user that there is a configuration file for irb. And finally I found the source for this new behavior on this github page.

As you might expect I do not like this change. For me, who is just learning both Ruby and Rails at the same time, I really appreciated the old behavior, when irb echoed the return value when I create a new object.
I gess you pros have forgotten how it is to learn a new language and a new developer environment?
What makes me sad is that @onlynone in one of the posts above asks:

"Would it be better if the default for echo_on_assignment was true so that no behavior changes unless one explicitly sets IRB.conf[:ECHO_ON_ASSIGNMENT] = false or uses --noecho-on-assignment ? I certainly prefer to suppress the echo of assignment expressions, but I can understand if we don't want behavior to change by default."

Spot on! As a newbie I can't argue against this new feature (from what I read you want Ruby more like Python-like, not echoing out hundreds of lines of code). My argument for letting the default value be RB.conf[:ECHO_ON_ASSIGNMENT] = true is that for a newbie like me, this output is very valuable. And it is really hard for a newbie to understand how to change this behavior in a configuration file that does not exist (I had to create a .irbrc file in my home directory).
Most newbies will never know that you actually can change the behavior of irb.

But for a developer proficient in Ruby irritated over lines of echoed code, it would probably easy to change the behavior to RB.conf[:ECHO_ON_ASSIGNMENT] = false, if he or she so preferred.

So for me this is a pedagogical question. What should be the default behavior? And how will it effect the users? A new user just learning ruby? An experienced programmer?

Please set the default value back to true! So you restore the previous behavior and help all new users that are learning Ruby (or Rails). If any user in time want to change the behavior of irb down the road, that person are experienced and most likely also know how to change these settings.

@gdonald
Copy link

gdonald commented Jun 18, 2020

It's spelled "newbie". https://www.merriam-webster.com/dictionary/newbie

@JorgenIvarsson
Copy link

@gdonald Thank you Greg for correcting my spelling.

@onlynone
Copy link
Contributor Author

My argument for letting the default value be RB.conf[:ECHO_ON_ASSIGNMENT] = true is that for a newbie like me, this output is very valuable. And it is really hard for a newbie to understand how to change this behavior in a configuration file that does not exist

If seeing the value from the return of a function is valuable, you don't have to look under the hood of irb settings to see it. You can just type the name of the variable you assigned the value at the prompt after the function call.

This change only comes into effect when you're doing an assignment like a = new_thing(). If you just type new_thing(), the returned value will still be echoed to the terminal. If you are executing an assignment statement, then by definition, you now have a reference to the thing you created, so you can just type a on the next line to see the value.

@JorgenIvarsson
Copy link

My argument for letting the default value be RB.conf[:ECHO_ON_ASSIGNMENT] = true is that for a newbie like me, this output is very valuable. And it is really hard for a newbie to understand how to change this behavior in a configuration file that does not exist

If seeing the value from the return of a function is valuable, you don't have to look under the hood of irb settings to see it. You can just type the name of the variable you assigned the value at the prompt after the function call.

This change only comes into effect when you're doing an assignment like a = new_thing(). If you just type new_thing(), the returned value will still be echoed to the terminal. If you are executing an assignment statement, then by definition, you now have a reference to the thing you created, so you can just type a on the next line to see the value.

Yes, I discovered that calls to the object echoed the value within minutes. The problem was that this behavior did not match the online guide I was following. This is a new behavior in Ruby from version 2.7. You have therefore, without warning, changed a behavior that has existed for many, many years. Of course, it can be argued that this changed behavior, in time, will be reflected in guides when they are being updated.

I would like to argue that this change was not asked for. If I read the original case (#11) there was a requested for a setting to be able to turn off the echo for assignments. That request is fulfilled by changing the configuration IRB.conf[:ECHO_ON_ASSIGNMENT] = false.

But instead of fulfilling that original request, you have changed a default behavior, so that all users of Ruby/Rails are affected by this new default value. I did not understand that there was such a request? If there is a general and strong desire to change this default setting in the ruby community, I must have missed it. If you can show that there is a strong request for this change of the default behavior, I will have to accept this change.
But if there isn’t a widespread request for this change, I would argue that this change is a setback for mainly new users of Ruby/Rails.

Another aspect of this is consistency. An important mantra for Rails is "Sensible defaults". Due to the changes in Ruby, we now get an incomprehensible and inconsistent behavior in the IRB. Previously, the rule was that you always got a return value echoed. Now we have a situation where calling a variable echo the value. When calling “new “, you do not get an echo of the return value. But If you call destroy, you get an echo of the return value in irb.

Again, I argue that as an experienced developer you can easily change these settings to your liking. But not so easy for the one who is just learning Rails. New developers will not know that they can change this behavior in a setting. As it now stands, this change doesn’t seem to be documented at all.

So instead of fulfilling the original request (to be able to turn off the default behavior), you have changed the general behavior for all users of Ruby/Ralls.
My humble opinion is that this was not the right way to do it. Please change it back.

@mtgrosser
Copy link

Please change the default back to what it has (always?) been. This behaviour change is totally unexpected and irritating, and I'm saying this as a longtime Rubyist.

In Ruby, everything evaluates to something. Till now, the philosophy of IRB adhered exactly to that idea. Hiding the value of an assignment is not POLS.

@aycabta aycabta mentioned this pull request Sep 3, 2020
@aycabta
Copy link
Member

aycabta commented Sep 13, 2020

I changed the behavior by #129.

Omit the results evaluated at assignment if they are too long.

The behavior of ECHO_ON_ASSIGNMENT being on by default is hard to understand, so I change it to off by default. Instead, we turn OMIT_ON_ASSIGNMENT on by default. The result is displayed on assignment, but it will always be short and within one line of the screen.

And I released irb gem 1.2.6 now. Please try it by gem install irb.

@aycabta
Copy link
Member

aycabta commented Sep 18, 2020

I created a concise Pull Request #130. (ref. #129 (comment))

Drop OMIT_ON_ASSIGNMENT and add :truncate option for ECHO_ON_ASSIGNMEN

And released 1.2.7.

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

Successfully merging this pull request may close these issues.

None yet