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 sort_by macro method #3947

Merged
merged 1 commit into from
Jan 30, 2019
Merged

Conversation

repomaa
Copy link
Contributor

@repomaa repomaa commented Jan 27, 2017

{{ ["abc", "a", "ab"].sort_by { |string| string.size } }}

Copy link
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

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

A documentation placeholder should be added at https://github.com/crystal-lang/crystal/blob/master/src/compiler/crystal/macros.cr#L543 .

I'm curious, which scenario forced you sort in macros?

@repomaa
Copy link
Contributor Author

repomaa commented Jan 27, 2017

@bcardiff done :) Sorry I forgot about that. I was writing a kind of model interface similar to json mapping and was building an initializer that would take arguments from the mapping named tuple. There i needed to sort the arguments by nilable so that args that are mandatory are leftmost.

There are other ways of doing it of course but i was confused that ArrayLiteral didn't have a sort_by method so i thought it would be nice to have.

src/compiler/crystal/macros/methods.cr Outdated Show resolved Hide resolved
@straight-shoota
Copy link
Member

@jreinert Could you rebase on master?

@repomaa
Copy link
Contributor Author

repomaa commented Jan 14, 2019

@straight-shoota: done!

@repomaa
Copy link
Contributor Author

repomaa commented Jan 28, 2019

@RX14 @straight-shoota: ping

@RX14 RX14 merged commit d58a06e into crystal-lang:master Jan 30, 2019
@RX14 RX14 added this to the 0.28.0 milestone Jan 30, 2019
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

5 participants