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

Feature: paste mode #56

Merged
merged 5 commits into from
Sep 2, 2017

Conversation

Porcupine96
Copy link
Contributor

No description provided.

@Porcupine96 Porcupine96 mentioned this pull request Aug 29, 2017
end

input += input_line
input += "\n"
Copy link
Member

@veelenga veelenga Aug 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strings in Crystal are immutable. So each time you do +=, you create a new string. Use String.build instead.

input += "\n"
end

if input.to_s.strip != ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!input.blank? should be a bit cleaner

@veelenga
Copy link
Member

veelenga commented Aug 30, 2017

Tested locally and it works:

icr(0.23.1) > |paste
# Entering paste mode (ctrl-D to finish)
puts "hello"
puts "bro"
[1, 2, 3].each { |e| puts e }
^D

# Ctrl-D was pressed, now interpreting...
hello
bro
1
2
3
 => nil

But I'm not sure if we need this because you can just use parenthesis (or begin, end):

icr(0.23.1) > (
icr(0.23.1) >   puts "hello"
icr(0.23.1) >   puts "bro"
icr(0.23.1) >   [1, 2, 3].each { |e| puts e }
icr(0.23.1) >   )
hello
bro
1
2
3
 => nil

@greyblake WDYT?

@Porcupine96
Copy link
Contributor Author

Your call guys. I made the requested changes :)

@veelenga
Copy link
Member

veelenga commented Aug 30, 2017

Personally I don't see advantages of |pcopy over parens or begin end...

@greyblake @crystal-community/maintainers thoughts?

@greyblake
Copy link
Member

I'll take a look tonight.

Copy link
Member

@greyblake greyblake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general looks good an works well.

I left a tiny comment regarding |paste -> paste.
What do other people think? @veelenga

@@ -28,11 +28,36 @@ module Icr
__exit__
elsif %w(exit quit).includes?(input.to_s.strip)
__exit__
elsif %w(|paste).includes?(input.to_s.strip)
Copy link
Member

@greyblake greyblake Aug 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it would be easier to read if it would be

input.to_s.strip == "|paste"

...

Also, I would prefer to use simply paste without |. Just like exit and quite commands above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greyblake good catch 👍

Using just paste means the user can't use this word as a variable or method name... But really rare case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the changes. paste doesn't seem like a good variable name anyway. ;)

input_line = Readline.readline()

if input_line.nil?
puts "\n\n# Ctrl-D was pressed, now interpreting...\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think interpreting is not the right word here because crystal is compiled language 😄

Maybe better to say: "Ctrl-D was pressed, exiting paste mode... "?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

@veelenga veelenga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall it looks good to me 👍 Just a small improvement...

@greyblake greyblake merged commit 4fe42cb into crystal-community:master Sep 2, 2017
veelenga added a commit that referenced this pull request Oct 7, 2017
veelenga added a commit that referenced this pull request Oct 7, 2017
veelenga added a commit that referenced this pull request Oct 9, 2017
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