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

Fix parsing of '!=' after an identifier #4834

Merged
merged 2 commits into from Aug 16, 2017
Merged

Conversation

oprypin
Copy link
Member

@oprypin oprypin commented Aug 15, 2017

Fixes #4815

@makenowjust
Copy link
Contributor

@makenowjust makenowjust commented Aug 16, 2017

I think it is breaking change.

@RX14
Copy link
Contributor

@RX14 RX14 commented Aug 16, 2017

Maybe it is a breaking change but the behaviour before was certainly a bug.

@bew
Copy link
Contributor

@bew bew commented Aug 16, 2017

So a method named method!= shouldn't work?

This PR breaks the following code:

class C
  def method!=(var)
    puts "assigning #{var}"
  end
end

C.new.method! = 4

Error:

Using compiled compiler at `.build/crystal'
Syntax error in test.cr:2: unexpected token: !=

  def method!=(var)
            ^

@RX14
Copy link
Contributor

@RX14 RX14 commented Aug 16, 2017

No, I don't think that code should work. It's easy enough to trim the ! off the property and use method = 4.

@ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Aug 16, 2017

Merging. What do bang (foo.bar! = 1) or question (foo.bar? = 2) setters bring to the language? IMHO only weirdness, if not confusion.

@ysbaddaden ysbaddaden merged commit e5b6efe into crystal-lang:master Aug 16, 2017
2 checks passed
@bew
Copy link
Contributor

@bew bew commented Aug 16, 2017

@RX14 @ysbaddaden I agree, it's just confusing. I just wanted to point it out, as it wasn't mentionned anywhere and this PR is breaking it.

@ysbaddaden ysbaddaden added this to the Next milestone Aug 16, 2017
@RX14
Copy link
Contributor

@RX14 RX14 commented Aug 16, 2017

@bew Thanks for mentioning it, i'm sure it'll make it into the changelog.

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

5 participants