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

Support custom implicit Function Arguments like PosInfos via abstract @:fromNothing macro #6616

Closed
wants to merge 16 commits into from

Conversation

frabbit
Copy link
Member

@frabbit frabbit commented Sep 27, 2017

No description provided.

let infos = mk_infos ctx p [] in
ordered_args @ [type_expr ctx infos (WithType t)]
else if ctx.com.config.pf_pad_nulls then
(ordered_args @ [(mk (TConst TNull) t_dynamic p)])
else
ordered_args
ordered_args @ [(mk (TConst TNull) t_dynamic p)]
Copy link
Member Author

@frabbit frabbit Sep 27, 2017

Choose a reason for hiding this comment

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

@ncannasse does this change, actually ignoring pf_pad_null for bind, does have any effects on real code? The unit tests still pass.

Copy link
Member

Choose a reason for hiding this comment

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

well it's not ignoring pf_pad_null but is always pushing nulls whereas you're padding or not. I'm not sure I like it, but I guess we can remove extra nulls at a later stage maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, true. I actually wonder if and what implications this actually have.

Copy link
Member Author

Choose a reason for hiding this comment

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

or to be more precise, which negative implications this have.

Copy link
Member Author

Choose a reason for hiding this comment

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

and just to let you know, there are already cases where this leads to bugs: http://try-haxe.mrcdk.com/#12d04.

@Simn
Copy link
Member

Simn commented Oct 9, 2017

You will have to rebase this one.

@frabbit
Copy link
Member Author

frabbit commented Nov 20, 2017

It's up to date again

@frabbit
Copy link
Member Author

frabbit commented Nov 28, 2017

@ncannasse @Simn @nadako any objections or remarks?

@nadako
Copy link
Member

nadako commented Nov 28, 2017

I'm yet to check this out and see what can I do with it :)

@Simn Simn added this to the Design milestone Apr 20, 2018
@ncannasse
Copy link
Member

I think fromNothing is badly named. I suggest using defaultValue() instead. Also, does it works if the method is not a macro as well ?

@Simn
Copy link
Member

Simn commented Oct 15, 2018

I think fromNothing is badly named. I suggest using defaultValue() instead. Also, does it works if the method is not a macro as well ?

I agree that it's badly named. But defaultValue is not much better. We should make it clear that this is only for function arguments so people don't assume that it's run on something like var x:Implicit<Blub>;.

@frabbit
Copy link
Member Author

frabbit commented Oct 15, 2018

@ncannasse

Also, does it works if the method is not a macro as well ?

Yes, it does atm, but its not very useful in my opinion because you don't know the expected type.

@back2dos
Copy link
Member

What about fields of object literals? Seems like a relatively good use case (will give you prop drilling for example). Would there be anything wrong with also using this for initializing variables?

@frabbit
Copy link
Member Author

frabbit commented Oct 17, 2018

@back2dos would love to see an example, because i fail to see how that could be done without having an abstract with a concrete underlying type.

@RealyUniqueName
Copy link
Member

Let's revisit this for 4.1

@RealyUniqueName RealyUniqueName modified the milestones: Design, Release 4.1 Sep 9, 2019
@Simn Simn assigned RealyUniqueName and unassigned ncannasse Feb 17, 2020
@Simn
Copy link
Member

Simn commented Feb 17, 2020

This branch would need an update. @RealyUniqueName Could you look into this?

@RealyUniqueName
Copy link
Member

Sure

@Simn Simn modified the milestones: Release 4.1, Release 4.2 Mar 13, 2020
@Simn Simn modified the milestones: Release 4.3, Later Mar 25, 2023
@Simn
Copy link
Member

Simn commented Feb 8, 2024

This PR is unfortunately unmergeable at this point. I'll link it to #4583 mostly because there are some good tests here, so if we add this feature for Haxe 5 we can add those in some way.

@Simn Simn closed this Feb 8, 2024
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.

6 participants