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

Extend choicesAllMatching to records. #3229

Merged
merged 12 commits into from
Sep 20, 2022

Conversation

HansOlsson
Copy link
Collaborator

Closes #3210
(Based on decision, and examples cleaned up to clarify what they actually show, since "it" isn't good enough.)

chapters/inheritance.tex Outdated Show resolved Hide resolved
chapters/inheritance.tex Outdated Show resolved Hide resolved
chapters/inheritance.tex Outdated Show resolved Hide resolved
chapters/inheritance.tex Outdated Show resolved Hide resolved
chapters/inheritance.tex Outdated Show resolved Hide resolved
chapters/inheritance.tex Outdated Show resolved Hide resolved
chapters/inheritance.tex Outdated Show resolved Hide resolved
Copy link
Collaborator

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

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

Mainly requesting that the big example is split into three smaller examples.

Other than that, this looks like an OK implementation of what the poll suggested in #3210 (comment). A language group meeting decision based on the poll would be nice, but I'm not sure we really need it for a small change like this when every participant in the poll was in favor.

HansOlsson and others added 6 commits September 6, 2022 23:27
Co-authored-by: Henrik Tidefelt <henrikt@wolfram.com>
Co-authored-by: Henrik Tidefelt <henrikt@wolfram.com>
Co-authored-by: Henrik Tidefelt <henrikt@wolfram.com>
Co-authored-by: Henrik Tidefelt <henrikt@wolfram.com>
Co-authored-by: Henrik Tidefelt <henrikt@wolfram.com>
chapters/inheritance.tex Outdated Show resolved Hide resolved
chapters/inheritance.tex Outdated Show resolved Hide resolved
chapters/inheritance.tex Outdated Show resolved Hide resolved
chapters/inheritance.tex Outdated Show resolved Hide resolved
HansOlsson and others added 4 commits September 9, 2022 10:14
Co-authored-by: Elena Shmoylova <eshmoylova@users.noreply.github.com>
Co-authored-by: Elena Shmoylova <eshmoylova@users.noreply.github.com>
Co-authored-by: Henrik Tidefelt <henrikt@wolfram.com>
And also specify that only recommend to present choices anyway for replaceable.
chapters/inheritance.tex Outdated Show resolved Hide resolved
Copy link
Collaborator

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

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

The breakdown of the example looks good now. I'm ready to provide an approving review as soon as I can point to a language group agreement to go with the poll in #3210 (comment).

@henrikt-ma henrikt-ma self-requested a review September 9, 2022 11:56
Co-authored-by: Henrik Tidefelt <henrikt@wolfram.com>
@HansOlsson
Copy link
Collaborator Author

The breakdown of the example looks good now. I'm ready to provide an approving review as soon as I can point to a language group agreement to go with the poll in #3210 (comment).

To me that poll is the language group agreement; as a simplified on-line poll in the language group where people have at least one week to participate (which is one of the alternatives in the work-flow).

@HansOlsson
Copy link
Collaborator Author

No objection during phone meeting.

@henrikt-ma
Copy link
Collaborator

To me that poll is the language group agreement; as a simplified on-line poll in the language group where people have at least one week to participate (which is one of the alternatives in the work-flow).

Even if the language group isn't asked to take a phone meeting decision based on the poll, I find the "polling comment" itself too unstable to be worth referencing. The least one could do would be to add another comment after the poll has been closed (in the same issue as the poll itself), summarizing the votes given in the poll and writing down a decision about how to proceed.

Copy link
Collaborator

@henrikt-ma henrikt-ma left a comment

Choose a reason for hiding this comment

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

I'll take #3229 (comment) as a note of a language group decision to go forward along with the poll result in #3210 (comment), where 5 out of 5 votes were In favor.

@HansOlsson HansOlsson merged commit 0f1ed88 into modelica:master Sep 20, 2022
@HansOlsson HansOlsson deleted the ExtendChoicesMatching branch September 20, 2022 19:10
@HansOlsson
Copy link
Collaborator Author

I'll take #3229 (comment) as a note of a language group decision to go forward along with the poll result in #3210 (comment), where 5 out of 5 votes were In favor.

Ok, will add such comments in the future.

@HansOlsson HansOlsson added the M36 For pull requests merged into Modelica 3.6 label Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M36 For pull requests merged into Modelica 3.6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Declare parameter records as replaceable?
3 participants