JoshCheek / seeing_is_believing

Displays the results of every line of code in your file
1.31k stars 54 forks source link

Outdated parser runtime dependency generates deprecation warning in Ruby 2.7 #155

Closed jmromer closed 4 years ago

jmromer commented 4 years ago

Using:

Output renders with the following deprecation warning:

~/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/parser-2.5.3.0/lib/parser/source/tree_rewriter.rb:269: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
~/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/parser-2.5.3.0/lib/parser/source/tree_rewriter/action.rb:16: warning: The called method `initialize' is defined here
p "hello world" # => "hello world"

# >> "hello world"

Parser may need to be updated.

jmromer commented 4 years ago

NB: This appears to be fixed on master / seeing_is_believing 3.6.2:

p "hello world" # => "hello world"

# >> "hello world"

Cutting a new release from master should suffice to resolve this issue.

brandoncc commented 4 years ago

@JoshCheek This breaks --xmpfilter-style:

brandoncc@Brandons-MBP ~/d/v/vscode-seeing-is-believing (master)> seeing_is_believing --xmpfilter-style test.rb
/Users/brandoncc/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0/gems/parser-2.5.3.0/lib/parser/source/tree_rewriter.rb:269: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/brandoncc/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0/gems/parser-2.5.3.0/lib/parser/source/tree_rewriter/action.rb:16: warning: The called method `initialize' is defined here
a = 1

brandoncc@Brandons-MBP ~/d/v/vscode-seeing-is-believing (master)> seeing_is_believing test.rb
/Users/brandoncc/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0/gems/parser-2.5.3.0/lib/parser/source/tree_rewriter.rb:269: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/brandoncc/.rbenv/versions/2.7.0/lib/ruby/gems/2.7.0/gems/parser-2.5.3.0/lib/parser/source/tree_rewriter/action.rb:16: warning: The called method `initialize' is defined here
a = 1  # => 1

Since --xmpfilter-style relies on stderr being empty, no update is done. Unfortunately, this is breaking my vscode extension.

Is there anything I can do to help get this fixed?

jmromer commented 4 years ago

@brandoncc As a temporary workaround until a new release is published, building and installing the gem from master resolves this issue:

gem build seeing_is_believing.gemspec  
gem install --local seeing_is_believing-3.6.2.gem  
% seeing_is_believing --xmpfilter-style test.rb
p "hello world"

# >> "hello world"
brandoncc commented 4 years ago

Thanks @jmromer, I also published an update to my extension which allows continuing when an error occurs. That fixed the extension when the new option is turned on.

JoshCheek commented 4 years ago

Sorry for the delay, y'all. I don't read my email and haven't been paying attention to GH lately.

I'd been holding off on a release since it's failing on Windows, and I don't have the tools necessary to debug it (my machine is just not powerful enough, so running it in a VM is excruciating). But, this issue seems more problematic (pretty sure the Windows issue is an issue with Childprocess exiting before all the children are cleaned up, which should hopefully not be a huge issue), so I'll just go ahead and go for it. If anyone reports issues on Windows (IIRC, around child process handling), I'll have to ask them to help me debug it 🤷‍

I bumped it to 4.0.0. The major version bump is because I had to update Childprocess from 2.0 to 3.0. However, since it's a major version bump, I took the opportunity to remove support for Ruby 2.3, which was causing a number of issues for me. It may still work (or mostly work) on Ruby 2.3, but I will no longer be supporting Ruby 2.3, just too much effort to adjust around all the little differences across all the versions.

Note that I didn't add tests around the latest syntax, and a quick test looks like it doesn't support some of it.

I'll have to add that in a followup release, don't really have the motivation to get to it right now. If anyone wants to do it for me, that'd be super rad 🙂 Tests would in spec/wrap_expressions_spec.rb and implementation in the case statement in SeeingIsBelieving::WrapExpressions#wrap_recursive You can see what the AST names you'll need like this:

image

Parser may need to be updated. Cutting a new release from master should suffice to resolve this issue. -- @jmromer

Updated it to 4.0 and cut a release, it should be fixed now. If not, tweet at me (I srsly rarely check email or GH) https://twitter.com/josh_cheek

Since --xmpfilter-style relies on stderr being empty, no update is done. Unfortunately, this is breaking my vscode extension.

Thanks @jmromer, I also published an update to my extension which allows continuing when an error occurs. That fixed the extension when the new option is turned on. -- @brandoncc

Sorry about that.

I didn't initially understand the example, but I think I do now: the VS Code extension assumes that nothing should be printed to stderr as this implies that implies something is wrong with SiB (as is currently the case), or with the extension. Was initially thinking we were talking about the user's stderr, but that will be be embedded within the script results.

I believe the 4.0.0 release I just cut should solve this.

Thinking on it a bit more, it might be worth just depending on the exit status and not the emptiness of stderr. Not totally sure that's a good idea, it does go against my natural tendencies of being extremely explicit WRT errors, but in this case, logging it would have allowed it to work without the user being impacted. Again, I'm not advocating that, just noting it (if it keeps happening that nonserious issues mess up the extension, then it might be pragmatic to switch to that approach).

brandoncc commented 4 years ago

This is awesome, thank you @JoshCheek!. There is no need to apologize for the issues with my extension, I added a workaround that allowed it to work.