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

Remove manual symbols in Crystal with automatic casting enums? #6736

Open
hanyuone opened this issue Sep 17, 2018 · 12 comments
Open

Remove manual symbols in Crystal with automatic casting enums? #6736

hanyuone opened this issue Sep 17, 2018 · 12 comments

Comments

@hanyuone
Copy link

I'm personally not sure if there are other examples, but #6427 could be easily replaced with an enum listing all the colours instead.

This has several advantages:

  • if a user enters in a wrong colour, the error is a compile-time error, not a runtime error
  • the code for colorize can be much cleaner

However, this might require an overhaul of the entire module since a lot of code in that module is dependent on the symbols themselves - for example, this section of code:

{% for name in COLORS %}
  def {{name.id}}
    @fore = ColorANSI::{{name.camelcase.id}}
    self
  end

  def on_{{name.id}}
    @back = ColorANSI::{{name.camelcase.id}}
    self
  end
{% end %}

As said in the beginning, I'm not sure if there are any other examples of enums being manually duplicated as symbols within the compiler, but if they are I think they should be converted fully to enums as well, to conform with #6074.

@hanyuone
Copy link
Author

hanyuone commented Oct 5, 2018

Bump: any updates?

@bcardiff
Copy link
Member

bcardiff commented Oct 5, 2018

I think that doing that would help to stress more the the auto casting of symbols to enum feature.

There are some corner cases that were reached in that PR and I think some intents to use the auto caste were reverted.

But I think the main usage of the autcasting for symbols to enum are in method calls, to avoid the user to type. The moment you try to assign it to variable, and pass it around it might do things harder to follow. Because, for example, in the opening comment snippet you might assume @fore is Symbol and not a ColorANSI.

@asterite
Copy link
Member

asterite commented Oct 5, 2018

I would personally revert the autocasting feature because it's incomplete (it breaks in the playground) and I can't think of a way to fix that.

@RX14
Copy link
Contributor

RX14 commented Oct 5, 2018

@asterite the problem is how the playground instruments code, not in autocasting.

@asterite
Copy link
Member

asterite commented Oct 5, 2018

I guess one way to fix it is to stop instrumenting literals (which is anyway not very useful because the value is literally there 😄). Yeah, my comment was a bit too much.

@hanyuone
Copy link
Author

hanyuone commented Oct 8, 2018

@bcardiff What exactly do you mean by the last paragraph?

@j8r
Copy link
Contributor

j8r commented Jan 4, 2019

I don't know for the removal of automatic casting in general, but at least converting auto-magically from Symbol to Enum is confusing:

  • we don't see any Symbol overload in the API docs, but we can still pass one instead of an enum
  • if we pass a random Symbol, the compiler even tell use we can't use symbols:
Error in line 2: no overload matches 'MyClass.new' with type Symbol
Overloads are: - MyClass.new(num : SomeEnum)
  • this may be a backslash for incremental compilation

@hanyuone
Copy link
Author

hanyuone commented Jan 4, 2019

@j8r I would have to disagree with some of your points.

For your first point, the fact that there’s no documentation doesn’t matter - symbols are a shorthand in this case, a part of Crystal’s design as opposed to being an overload to a function. Documentation in this case would make more sense in something like the Crystal guidebook.

For your second point, this could be fixed by something like adding an extra section to the error message:

Instead of “:foo”, do you mean:

- :foop (Bar::Foop)
- :foob (Baz::Foob)
- :goo (Quux::Goo)

Another way this could be fixed is by using a special syntax for this type of shorthand, maybe something like :.foo or :foo:, but it’d increase the learning curve and implementation complexity of this feature.

@j8r
Copy link
Contributor

j8r commented Jan 4, 2019

How a beginner is suppposed to know this @Qwerp-Derp ? I find this feature to obfuscate code comprehension: the enum is replaced by a symbol: MyEnum::Val => :val – we lost MyEnum.
This yields to methods accepting symbols instead of an enum.

@straight-shoota
Copy link
Member

straight-shoota commented Jan 4, 2019

This feature needs to be documented in the language reference. But only when we're certain about if and how it should stay. Right now, it should be considered experimental, hence the missing documentation. We want to try it out and see how it works while discussing additional features. For example there is a proposal to remove symbols entirely from the language, which would make the current symbol syntax exclusively used to enum autocasts.

@Qwerp-Derp Your idea to improve the error message sounds great. Would you mind opening a separate issue for that?

This yields to methods accepting symbols instead of an enum.

No symbol is ever accepted by this feature. It's only literal symbol syntax that is expanded to the full enum member name when it matches one that fits. It's completely type safe.

this may be a backslash for incremental compilation

There is no difference on the semantic level between Enum::Foo and :foo.

@j8r
Copy link
Contributor

j8r commented Jan 4, 2019

@straight-shoota If I've properly understood, SymbolLiteral could be replaced with a StringLiterral (even CharLiteral) then?

@RX14
Copy link
Contributor

RX14 commented Jan 4, 2019

All the issues with enum shorthands are solved by removing symbols. And enum shorthands are more useful than symbols.

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

Successfully merging a pull request may close this issue.

7 participants