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

Removing Modifications (MCP-0009) #3167

Merged
merged 22 commits into from
Nov 14, 2022
Merged

Removing Modifications (MCP-0009) #3167

merged 22 commits into from
Nov 14, 2022

Conversation

qlambert-pro
Copy link
Collaborator

Creating the pull request to start discussions

HansOlsson and others added 12 commits March 17, 2021 11:39
@HansOlsson
Create README.md
07e868b 
Add from old svn-server
@HansOlsson
Update README.md
d57a2aa 
Clarified.
@HansOlsson
Update README.md
5428481 
Added experience in Dymola.
@HansOlsson
Update README.md
ef0ae74 
Added user experience
@HansOlsson
Update README.md
ae047f1
@HansOlsson
Update README.md
34dfa49 
As discussed at the meeting.
@HansOlsson
Merge pull request #2936 from modelica/HansOlsson-patch-1
c95bd47 
Update README.md for balanced models.
@HansOlsson
Update README.md
9e21d2c 
Clarified experience.
@HansOlsson
Update README.md
33ef32f 
Removed empty-variant. The meeting otherwise found it ok.
@HansOlsson
Update README.md
5abfbcb
@HansOlsson
Merge remote-tracking branch 'upstream/master' into MCP/0009
203c432
@HansOlsson
Add semantics of break.
ecc13b8
qlambert-pro
qlambert-pro commented May 9, 2022
View reviewed changes
Copy link
Collaborator Author

@qlambert-pro qlambert-pro left a comment •
edited
Loading

Choose a reason for hiding this comment

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

So I can't formally approve the PR because I created it. But it looks good to me.

eshmoylova
eshmoylova requested changes May 10, 2022
View reviewed changes
chapters/inheritance.tex Outdated Show resolved Hide resolved
chapters/inheritance.tex Outdated Show resolved Hide resolved
chapters/inheritance.tex Show resolved Hide resolved
eshmoylova
eshmoylova reviewed May 12, 2022
View reviewed changes
chapters/inheritance.tex Outdated Show resolved Hide resolved
chapters/inheritance.tex Show resolved Hide resolved
chapters/inheritance.tex Outdated Show resolved Hide resolved
@HansOlsson @eshmoylova
Update chapters/inheritance.tex
a1ef806 
Co-authored-by: Elena Shmoylova <eshmoylova@users.noreply.github.com>
@henrikt-ma henrikt-ma changed the title Mcp/0009 Removing Modifications (MCP-0009) May 17, 2022
HansOlsson
HansOlsson approved these changes May 23, 2022
View reviewed changes
Copy link
Collaborator

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

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

Seems ok to me.
(Yes, I actually made the changes - so a bit weird; but reverse of qlambert - so one approval from us two should count).

@HansOlsson HansOlsson added the MCP Generic MCP label (prefer specific MCP label for grouping of issues belonging to the same MCP) label May 23, 2022
@perost
Copy link
Collaborator

perost commented Jun 15, 2022

Seems ok to me too. We haven't implemented it in OpenModelica yet, but it looks fairly straight-forward and I don't really see any issues.

@HansOlsson HansOlsson self-requested a review September 14, 2022 13:27
HansOlsson
HansOlsson approved these changes Sep 14, 2022
View reviewed changes
Copy link
Collaborator

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

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

Expanding on my previous approval.
I didn't actually author the MCP - I just moved it forward; and converted it to markdown.

The experience with the proposal is that it is a small addition, easy to implement, and easy to understand. The implementation effort was less than two days - including error cases; and handling the various cases in the GUI. The only minor issue is that some attributes are connected (so breaking start while keeping fixed=true), but the same applies in other cases and a tool can help reduce that problem - and I don't see how to easily fix it.

@HansOlsson
Copy link
Collaborator

Collection of reviews:

  • Hans Olsson: "The experience with the proposal is that it is a small addition, easy to implement, and easy to understand. The implementation effort was less than two days - including error cases; and handling the various cases in the GUI. The only minor issue is that some attributes are connected (so breaking start while keeping fixed=true), but the same applies in other cases and a tool can help reduce that problem - and I don't see how to easily fix it." Removing Modifications (MCP-0009) #3167 (review)_
  • Quentin Lambert: "So I can't formally approve the PR because I created it. But it looks good to me."; Removing Modifications (MCP-0009) #3167 (review)_
  • Per Öst: "Seems ok to me too. We haven't implemented it in OpenModelica yet, but it looks fairly straight-forward and I don't really see any issues." Removing Modifications (MCP-0009) #3167 (comment)_

Some remaining issue that seems to be resolved, and as soon as that is confirmed as resolved we can move forward.

eshmoylova
eshmoylova requested changes Sep 29, 2022
View reviewed changes
chapters/inheritance.tex Outdated Show resolved Hide resolved
chapters/inheritance.tex Outdated Show resolved Hide resolved
HansOlsson and others added 2 commits September 29, 2022 15:52
@HansOlsson @eshmoylova
Update chapters/inheritance.tex
3735731 
Co-authored-by: Elena Shmoylova <eshmoylova@users.noreply.github.com>
@HansOlsson @eshmoylova
Update chapters/inheritance.tex
50c5b92 
Co-authored-by: Elena Shmoylova <eshmoylova@users.noreply.github.com>
@HansOlsson HansOlsson added this to the MCP-2-UnderEvaluation milestone Sep 30, 2022
@HansOlsson HansOlsson merged commit ae5f432 into master Nov 14, 2022
@HansOlsson HansOlsson deleted the MCP/0009 branch November 14, 2022 21:47
@HansOlsson HansOlsson mentioned this pull request Nov 21, 2022
Closed
@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
Reviewers

@dietmarw dietmarw dietmarw left review comments

@HansOlsson HansOlsson HansOlsson approved these changes

@henrikt-ma henrikt-ma henrikt-ma left review comments

@eshmoylova eshmoylova eshmoylova approved these changes

Assignees
No one assigned
Labels
M36 For pull requests merged into Modelica 3.6 MCP Generic MCP label (prefer specific MCP label for grouping of issues belonging to the same MCP)
Projects
None yet
Milestone
MCP-2-UnderEvaluation
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@qlambert-pro @perost @HansOlsson @dietmarw @eshmoylova @henrikt-ma