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

Add \a escape sequence #5864

Merged
merged 33 commits into from May 11, 2018
Merged

Add \a escape sequence #5864

merged 33 commits into from May 11, 2018

Conversation

wooster0
Copy link
Contributor

@wooster0 wooster0 commented Mar 25, 2018

The escape sequence that beeps/bells.

Right now when you do

puts "\x07"

then you can hear a beep/bell and it already works because 07 is the HEX value in ASCII of \a.
But I still think that we should add the escape sequence \a directly because it's really some kind of special feature. Thats pretty much the only thing you can hear in a terminal.

This escape sequence can be useful for example when you are making an timer or something like that. So you can notify the user while he's doing something else. And it can be also helpful at debugging something without a display. Or generally debugging.

The escape sequence that makes a beep.
@oprypin
Copy link
Member

oprypin commented Mar 25, 2018

I think when the original developer was deciding for the first time what escape sequences to include, they made an educated decision on which ones are important. I don't think it's a matter of forgetting to include it or anything like that. More is not always better. Ruby having it is not a good argument.

@oprypin
Copy link
Member

oprypin commented Mar 25, 2018

Now that I look at it more carefully, this does seem like an unusual omission, because Crystal has all other escape sequences from these two

@@ -230,6 +230,7 @@ describe "Lexer" do
it_lexes_number :i8, ["0i8", "0"]

it_lexes_char "'a'", 'a'
it_lexes_char "'\\a'", '\a'
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 to keep release compatibility you would have to write 7.chr not '\a'

@@ -493,6 +493,8 @@ module Crystal
case char
when '\\'
case char = next_char
when 'a'
io << "\x07"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd keep "\u{7}" format.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah basically you have to do whatever the \b addition does. New source code must still compile with the old compiler.

@@ -1789,6 +1793,8 @@ module Crystal
end
else
case char = next_char
when 'a'
string_token_escape_value "\x07"
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@wooster0
Copy link
Contributor Author

wooster0 commented Mar 26, 2018

Ok now I did whatever the \b addition does. In every file. I searched through every single file for \b.

But CircleCI says:

Syntax error in src/string.cr:3942: invalid char escape sequence



      when '\a' then io << "\\a"

           ^

@oprypin
Copy link
Member

oprypin commented Mar 26, 2018

Can't use \a anywhere, except when it's \\a

@oprypin
Copy link
Member

oprypin commented Mar 26, 2018

@wooster0 wooster0 closed this Apr 5, 2018
@Sija
Copy link
Contributor

Sija commented Apr 5, 2018

@RX14 \b is unprintable as well, I see no reason to skip this one, while having all others included. This should be added only if for completeness sake.

@j8r
Copy link
Contributor

j8r commented Apr 5, 2018

Agree, there are also some languages that support it like Ruby and Python, and others don't like Rust and Go AFAIK.

@straight-shoota
Copy link
Member

Go supports \a, Rust does not.

@RX14
Copy link
Contributor

RX14 commented Apr 6, 2018

@Sija we should remove \b.

@RX14
Copy link
Contributor

RX14 commented Apr 6, 2018

It seems rust supports only \u{abcde} (only with curly braces!), \xAB, and then \n, \r, \t, and \0. And obviously \\. We should duplicate that.

@Sija
Copy link
Contributor

Sija commented Apr 6, 2018

@RX14 or add \a...

@RX14
Copy link
Contributor

RX14 commented Apr 6, 2018

\a is platform-specific and obsolete, so is \b. Should we add \f? \v? \e too? No, let's support only the common subset and everything else can be done just fine with \u{}.

@straight-shoota
Copy link
Member

\b\f\v\e are actually recognized (see https://crystal-lang.org/docs/syntax_and_semantics/literals/string.html ). I think we all agree we should either remove all of them or add \a. I don't have a strong position on this. They are probably mostly useless - but on the other hand it doesn't really hurt to have them and in rare instances it might help interoprability.

@RX14 How is \a platform-specific? It's just an alternative name to express codepoint 7.

@RX14
Copy link
Contributor

RX14 commented Apr 6, 2018

I absolutely agree we should add all of them or remove all of them except r, n, t. I just think we should remove complexity, and arcane, typically unused escape sequences.

@straight-shoota
Copy link
Member

I'd say \e should also stay. It is even used in Colorize and Benchmark::IPS

@Sija
Copy link
Contributor

Sija commented Apr 6, 2018

@RX14 What's the point of removing well-known functionalities (even if they indeed have arcane heritage), just for removal sake? Wouldn't be better to stay compatible with other languages and be modern in ways that are meaningful?

@asterite
Copy link
Member

asterite commented Apr 6, 2018

I didn't include "\a" because I don't find it useful. I wouldn't mind having it in the language, though.

@RX14
Copy link
Contributor

RX14 commented Apr 6, 2018

I don't care enough to bikeshed over it. If this many people feel it should be in the language, then just add it and I won't use it. Either way this PR (clearly done from the github web UI) wasn't very good.

@wooster0 wooster0 reopened this Apr 7, 2018
@asterite
Copy link
Member

(if someone approves this and wants to merge this, please squash :-)

@Sija
Copy link
Contributor

Sija commented Apr 28, 2018

Also, json additions need to be removed, since \a is invalid character in JSON string.

@@ -1444,6 +1444,7 @@ describe "String" do
"a".dump.should eq %("a")
"\\".dump.should eq %("\\\\")
"\"".dump.should eq %("\\\"")
# TODO: "\a".dump.should eq %("\\a")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a note uncomment after 0.24.2?

Copy link
Contributor Author

@wooster0 wooster0 Apr 28, 2018

Choose a reason for hiding this comment

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

I think this is enough. Right after 0.25.0 gets released, I will finish this with another PR and there I will just look here to see what I need to do.

@@ -493,6 +493,8 @@ module Crystal
case char
when '\\'
case char = next_char
when 'a'
io << "\u{7}"
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason these were not touched by #5882 @Sija?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, just an omission.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I make a PR to also make these strings chars?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to merge this one first.

@sdogruyol sdogruyol merged commit 1ecc65b into crystal-lang:master May 11, 2018
@sdogruyol sdogruyol added this to the Next milestone May 11, 2018
chris-huxtable pushed a commit to chris-huxtable/crystal that referenced this pull request Jun 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants