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

Macro map_with_index #8049

Merged

Conversation

Blacksmoke16
Copy link
Member

@Blacksmoke16 Blacksmoke16 commented Aug 6, 2019

  • Implements the map_with_index method on TupleLiteral and ArrayLiteral.
  • Reorganizes the macro method specs a bit
    • Alphabetizes each type
    • Use the constant type vs string
    • Updates some type's structure to use the standard format
      • Mainly just TupleLiteral and ArrayLiteral.
    • Marks some missing tests as pending
      • Can make another PR later to fill these in.

Reorganization will be handled in another PR.

Use case is something like (pseudo code):

initializer = some_class_def.methods.find(&.name.==("initialize"))
service_ann.args.map_with_index { |arg, idx| arg.as(initializer.args[idx].restriction) } 

@straight-shoota
Copy link
Member

straight-shoota commented Aug 6, 2019

Could you please separate reorganization and feature addition into separate commits (or maybe even PRs)? Thanks!

@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented Aug 6, 2019

@straight-shoota I'll make another PR with the reorganization plus implementing the missing specs. This should be good to review now.

spec/compiler/macro/macro_methods_spec.cr Outdated Show resolved Hide resolved
src/compiler/crystal/macros/interpreter.cr Outdated Show resolved Hide resolved
src/compiler/crystal/macros/methods.cr Outdated Show resolved Hide resolved
src/compiler/crystal/syntax/ast.cr Outdated Show resolved Hide resolved
@@ -422,6 +422,10 @@ module Crystal::Macros
def gsub(regex : RegexLiteral, replacement : StringLiteral) : StringLiteral
end

# Returns a `StringLiteral` where all `:` are replaced with `_`.
def identify : StringLiteral
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be another PR?

@RX14
Copy link
Contributor

RX14 commented Aug 18, 2019

What is the StringLiteral#identify macro method useful for?

@Blacksmoke16
Copy link
Member Author

@RX14 No idea, it already exists but just wasn't documented.

@Blacksmoke16
Copy link
Member Author

Bump, this should be good to go.

@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented Sep 22, 2019

@RX14 @bcardiff @straight-shoota Any chance of this making it in the next release?

@Blacksmoke16
Copy link
Member Author

Anything left to do here?

@straight-shoota
Copy link
Member

straight-shoota commented Oct 31, 2019

This needs to be washed into two commits, one focused on adding map_with_index and the other containing the unrelated changes.

@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented Nov 3, 2019

@straight-shoota Done. Just made one commit with the changes. Can make another PR later for adding that macro method to the doc list.

@straight-shoota
Copy link
Member

Yes, please. It should be a separate commit, either in this PR or in another one.

@straight-shoota straight-shoota merged commit 5a5704c into crystal-lang:master Nov 13, 2019
@straight-shoota straight-shoota added this to the 0.32.0 milestone Nov 13, 2019
@Blacksmoke16 Blacksmoke16 deleted the macro-map-with-index branch November 13, 2019 13:00
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.

3 participants