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.

Sign up for GitHub

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

Jump to bottom

Parameter assignment syntax #174

Merged
merged 6 commits into from
Jan 28, 2023
Merged

Parameter assignment syntax #174

merged 6 commits into from
Jan 28, 2023

Conversation

jemc
Copy link
Member

@jemc jemc commented May 25, 2020 •
edited
Loading

This PR adds my RFC for assign param syntax.

@jemc jemc self-assigned this May 25, 2020
@jemc jemc mentioned this pull request May 25, 2020
Open
@Theodus Theodus added the status - new The RFC is new and ready for discussion. label Jun 14, 2020
@ponylang-main ponylang-main mentioned this pull request Jun 14, 2020
Closed
@aturley
Copy link
Member

aturley commented Jun 16, 2020

Discuss more next week.

Base automatically changed from master to main February 8, 2021 22:15
@SeanTAllen SeanTAllen added the discuss during sync Should be discussed during an upcoming sync label Feb 9, 2022
@SeanTAllen SeanTAllen removed the discuss during sync Should be discussed during an upcoming sync label Feb 22, 2022
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Feb 24, 2022
@SeanTAllen SeanTAllen removed the discuss during sync Should be discussed during an upcoming sync label Mar 1, 2022
@SeanTAllen
Copy link
Member

@jemc how would you feel about moving forward with this. instead of @? I'd like to move this and #173 forward but we have some syntax issues to resolve. I'm in favor of "this" because it is the least breaking option.

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Jan 2, 2023
@jemc
Copy link
Member Author

jemc commented Jan 3, 2023

I'd agree that this. is "better than nothing" for the purposes of this RFC, so I'd be good with amending this RFC to use that syntax. It's still less boilerplate than the current status quo.

For #173 I would not be in favor of making that change with this. - I think it's too much friction added for something so ubiquitous.

@jemc
Update "assign param" RFC to use this. syntax.
12a15da 
The support for the `@` syntax is not there yet, so we can move forward with this RFC without that syntax yet in place.
@SeanTAllen
Copy link
Member

We appear to have an impasse on #173, but it would be good to move forward with this RFC so let's do that and continue to discuss #173.

jasoncarr0
jasoncarr0 reviewed Jan 3, 2023
View reviewed changes
text/0000-assign-param-syntax.md
new val create(name: String, this.return_type, this.params, this.attachments = None) =>
this.name = Id(name)
this.partial = None
this.guard = None
Copy link

Choose a reason for hiding this comment

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

What happens with this PR if the body of the constructor is now empty? Do we still need to include => None?

Copy link
Member

Choose a reason for hiding this comment

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

My expectation would be yes, you need to. I think it is worth explicitly noting that in this RFC.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated it to explicitly mention that requirement.

@SeanTAllen SeanTAllen removed the status - new The RFC is new and ready for discussion. label Jan 3, 2023
@jasoncarr0
Copy link

jasoncarr0 commented Jan 3, 2023 •
edited
Loading

One more note possibly worth adding: are any names supported for keyword argument syntax? Does that create semver issues with the name requirements?

@SeanTAllen
Copy link
Member

It was noted on the sync call that @jemc will note that how this interacts with named arguments in unresolved questions.

@jemc jemc added the status - final comment period The RFC is finalized. Waiting for final comments. label Jan 10, 2023
@ponylang-main ponylang-main mentioned this pull request Jan 10, 2023
Closed
@SeanTAllen SeanTAllen changed the title Add RFC for assign param syntax. Parameter assignment syntax Jan 10, 2023
@jemc jemc added status - ready for vote The RFC is ready to be voted on. and removed status - final comment period The RFC is finalized. Waiting for final comments. labels Jan 24, 2023
@ponylang-main ponylang-main mentioned this pull request Jan 24, 2023
Closed
@SeanTAllen
Copy link
Member

This was accepted during sync. I will do the merge and what not later.

@SeanTAllen SeanTAllen assigned SeanTAllen and unassigned jemc Jan 24, 2023
@SeanTAllen SeanTAllen merged commit 6bc873c into main Jan 28, 2023
@SeanTAllen SeanTAllen deleted the assign-param-syntax branch January 28, 2023 17:55
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Jan 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SeanTAllen SeanTAllen SeanTAllen left review comments

@jasoncarr0 jasoncarr0 jasoncarr0 approved these changes

Assignees

@SeanTAllen SeanTAllen

Labels
status - ready for vote The RFC is ready to be voted on.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jemc @aturley @SeanTAllen @jasoncarr0 @Theodus @ponylang-main