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

Allow deleting elements from NamedTuple/Hash literals #9837

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Blacksmoke16
Copy link
Member

Resolves #8849

@Blacksmoke16
Copy link
Member Author

This is good for another review.

@Blacksmoke16
Copy link
Member Author

@asterite Done. Also rebased on master to fix that CI issue.

@Blacksmoke16
Copy link
Member Author

This is good for another review.

@Blacksmoke16
Copy link
Member Author

Bump.

@Blacksmoke16
Copy link
Member Author

@oprypin Bump :)

@HertzDevil
Copy link
Contributor

Does #7109 (comment) not apply here?

@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented May 31, 2021

@HertzDevil I'm not sure. IMO it makes sense to have this given you can construct a hash entirely within macro land, but then if you want to remove a key from it, you can't and would have rebuild the whole hash. So it's not like you cant mutate a hash at the moment, it's just incomplete.

That's my usecase at least, copying the named arguments from an annotation, processing it (i.e. using the provided values to add/remove arguments), then double splat it into the constructor of a struct.

@oprypin
Copy link
Member

oprypin commented May 31, 2021

@Blacksmoke16 I'm not sure what you mean. Maybe it makes sense, but the argument against deleting from a Hash is exactly the same as about deleting from an Array (the linked comment)

@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented May 31, 2021

@oprypin I don't see it that way. The way I took it the linked comment's argument is against mutations because it makes tracking dependency order harder. I.e. using the old mutable constant DSL pattern before annotations became more prevalent.

However after working a lot more with macros since that original issue; I don't think that argument is entirely correct mainly because there are use cases for mutating data structures other than as a means of "registering" things. This is even more important if/when #8835 is ever implemented, as it's easy to see hashes/arrays being passed that you'd expect to be able to modify.

I also don't really see the benefit of removing mutating methods from these macro literals anyway. It just seems like it's making them less useful for no reason.

EDIT: I do agree that there are features that could be implemented as a better/easier alternative, e.g. #9802, but removing these methods still seems kinda arbitrary.

@Blacksmoke16
Copy link
Member Author

Bump, ran into this again.

I have an annotation like @[ADI::Register(tags: [{name: "baz", value: 123, tag_type: CustomTag}])]. Each tag can have an optional tag_type that maps this tag to a specific struct. E.g:

struct CustomTag < ADI::AbstractTag
  getter value : Int32

  def initialize(@value : Int32, name : String, priority : Int32? = nil, *, tag_type = nil)
    super name, priority
  end
end

Of which I'm creating it via "#{tag[:tag_type]}.new(#{tag.double_splat})".id, but since I can't delete tag_type from the annotation's named args, I have to include tag_type in the initializer of the struct so that it doesn't error due to no matching overload.

@HertzDevil
Copy link
Contributor

IMO it makes more sense to have HashLiteral#select, which ArrayLiteral also has, and then we could write tag.select { |k, v| k != :tag_type }.double_splat, without introducing a new source of mutability.

(I would personally avoid this call and double_splat altogether, manually iterate over tag, and interpolate each element separately.)

@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented Aug 14, 2021

@HertzDevil Not as elegant but that would work too I suppose. Better than nothing.

manually iterate over tag, and interpolate each element separately.

That's a bit easier said than done I think because the instantiation of the struct is still in the macro context. If it wasn't then you could easily use a for in loop to do it.

EDIT: Looks like you could do .to_a.filter.each to reconstruct it, is better than needing the arg in the constructor at least.
EDIT2: Er, maybe not given you can't construct a named tuple, and HashLiteral#double_splat is kinda meh, ref: #8266. Just need to create the named tuple via tag_args = {name: ""}.

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.

Implement HashLiteral#delete and NamedTupleLiteral#delete
7 participants