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

replace mapping with serializable in compiler #6308

Merged

Conversation

kostya
Copy link
Contributor

@kostya kostya commented Jul 1, 2018

No description provided.

Copy link
Contributor

@RX14 RX14 left a comment

Choose a reason for hiding this comment

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

Most of these are structs - can some of the properties be getters instead?

@kostya
Copy link
Contributor Author

kostya commented Jul 2, 2018

travis crashed with getters:

in src/compiler/crystal/tools/expand.cr:122: undefined method 'expansions=' for Crystal::ExpandResult
        res.expansions = @found_nodes.map { |node| ExpandResult::Expansion.build(node) }

@straight-shoota
Copy link
Member

Just use getters where no setters are needed. You don't have to wait for travis if you run the specs locally.

@kostya
Copy link
Contributor Author

kostya commented Jul 2, 2018

i don't know how it used and where, with mapping it all was as property.

@straight-shoota
Copy link
Member

Well, it's just six ivars and you already know one needs a setter. Just trial and error will get you through very quickly.

@asterite
Copy link
Member

asterite commented Jul 2, 2018

But what's the problem? They were properties before. I'd say that can be "fixed" in another PR...

Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

Thank you @kostya 👍

@sdogruyol sdogruyol merged commit d36a654 into crystal-lang:master Jul 28, 2018
@sdogruyol sdogruyol added this to the Next milestone Jul 28, 2018
@asterite
Copy link
Member

Next we should remove mapping.

@RX14 RX14 modified the milestones: Next, 0.26.0 Jul 30, 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

6 participants