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 EOLconstant (End-Of-Line) #11303

Merged
merged 4 commits into from
Nov 10, 2023

Conversation

postmodern
Copy link
Contributor

@postmodern postmodern commented Oct 11, 2021

  • Added EOL to src/kernel.cr.
  • Is OS aware ("\r\n" on Windows, '\n' otherwise).

Resolves #10885

…ng#10885).

* Is OS aware ("\r\n" on Windows, '\n' otherwise).
@postmodern
Copy link
Contributor Author

postmodern commented Oct 11, 2021

Some additional thoughts:

  • Having EOL be a String on Windows, but a Char elsewhere might cause some unexpected type issues?
  • Should we also add explicit CR, LF and CRLF constants? HTTP also uses CRLF.
  • Is the top-level really the best place for such String constants?

@straight-shoota
Copy link
Member

Having EOL be a String on Windows, but a Char elsewhere might cause some unexpected type issues?

Yes, it should be the same type on all platforms (cf #10645). String is a perfect match for this.

Should we also add explicit CR, LF and CRLF constants? HTTP also uses CRLF.

I don't think that's necessary. Libraries like HTTP can of course define such constants as they need them.

Is the top-level really the best place for such String constants?

String::EOL or System::EOL might be viable alternatives. I might fancy the latter because it emphasizes that the constant's value is system-specific. Not settled on that, though.

@asterite
Copy link
Member

What is the use case of this constant?

@straight-shoota
Copy link
Member

@asterite The proposal is in #10885

@oprypin oprypin changed the title Added the top-level EOL (End-Of-Line) constant (implements #10885). Added the top-level EOL (End-Of-Line) constant Oct 11, 2021
@Blacksmoke16
Copy link
Member

Blacksmoke16 commented Oct 18, 2021

👍 for System::EOL. I checked a few other langs and they all seem to have it in systemish namespaces:

@postmodern
Copy link
Contributor Author

Do we have consensus on System::EOL vs String::EOL?

@j8r
Copy link
Contributor

j8r commented Nov 7, 2021

I may be alone, I still prefer NEWLINE (in whatever namespace it lands). Already quite small and more explicit than EOL.

@Sija
Copy link
Contributor

Sija commented Nov 7, 2021

@j8r If I'd like to write code that would be read by my grandma I'd agree with you for sure, but most programmers I know have a pretty firm notion of EOF, EOL, and other abbreviations from the programming world... ;)

@j8r
Copy link
Contributor

j8r commented Nov 7, 2021

Anyway, can this constant be useful in the stdlib, and even the compiler?

@HertzDevil
Copy link
Contributor

private def newline
{% if flag?(:win32) %}
"\r\n"
{% else %}
"\n"
{% end %}
end

@Blacksmoke16
Copy link
Member

System::EOL is still my vote. Just ran into a use case where this would be helpful.

src/kernel.cr Outdated Show resolved Hide resolved
@HertzDevil
Copy link
Contributor

Another one:

private def newline
{% if flag?(:win32) %}
"\r\n"
{% else %}
"\n"
{% end %}
end

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

LGTM.

@straight-shoota straight-shoota changed the title Added the top-level EOL (End-Of-Line) constant Add EOLconstant (End-Of-Line) Nov 8, 2023
@straight-shoota straight-shoota added this to the 1.11.0 milestone Nov 8, 2023
Comment on lines +93 to +97
EOL = {% if flag?(:windows) %}
"\r\n"
{% else %}
"\n"
{% end %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be shortened into:

Suggested change
EOL = {% if flag?(:windows) %}
"\r\n"
{% else %}
"\n"
{% end %}
EOL = {{ flag?(:windows) ? "\r\n" : "\n" }}

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 find ternary statements overly confusing. They only help reduce line count at the cost of readability.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose you could also write it as {{ if flag?(:windows) "\r\n" else "\n" end }} if that's more approachable?
Anyway, these are all equivalent expressions of the same thing and there's no clear preference for one or the other. So let's keep it as is.

@straight-shoota straight-shoota merged commit 21816db into crystal-lang:master Nov 10, 2023
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
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.

Add an OS-aware newline constant
7 participants