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

Clean-up of near-zero coordinates in graphical annotations #4107

Merged
merged 12 commits into from
May 24, 2023

Conversation

DagBruck
Copy link
Contributor

@DagBruck DagBruck commented Apr 5, 2023

Minor clean-up of near-zero coordinates in graphical annotations.

@CLAassistant
Copy link

CLAassistant commented Apr 5, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ tobolar
❌ Dag Brück


Dag Brück seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@TManikantan TManikantan linked an issue Apr 18, 2023 that may be closed by this pull request
@TManikantan
Copy link
Contributor

@DagBruck can you please make changes suggested by @tobolar . Many Thanks

@TManikantan TManikantan added this to the MSL4.1.0 milestone Apr 21, 2023
@arunkumar-narasimhan
Copy link
Contributor

@DagBruck, would you like our help in addressing @tobolar's comments? These are minor suggestions that we could also help close if you are stressed for time.

@tobolar tobolar changed the title Dag bruck 01 Clean-up of near-zero coordinates in graphical annotations May 5, 2023
@tobolar
Copy link
Contributor

tobolar commented May 5, 2023

would you like our help in addressing @tobolar's comments?

It's not too much effort to change the code on our own and close the PR. But the reason why not to do it is that the Dymola's command/script shall be improved to catch also doubled coordinates in annotations.

I have fixed and commented some comments of me occuring several times. So only comments significant for the Dymola script's change are left.

@arunkumar-narasimhan
Copy link
Contributor

would you like our help in addressing @tobolar's comments?

It's not too much effort to change the code on our own and close the PR. But the reason why not to do it is that the Dymola's command/script shall be improved to catch also doubled coordinates in annotations.

I have fixed and commented some comments of me occuring several times. So only comments significant for the Dymola script's change are left.

Thanks for the commits, @tobolar. I didn't want to overstep assignee's work that too without at least notifying the assignee. Appreciate the info on Dymola command/script feature. We will probe for second review.

@TManikantan
Copy link
Contributor

@tobolar @hubertus65 kindly review the fix. This was necessary as new commits were made. Many Thanks...

Copy link
Contributor

@tobolar tobolar left a comment

Choose a reason for hiding this comment

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

kindly review the fix

IMO, there are two options

  1. fix my comments and merge or
  2. wait for @DagBruck response.

@MartinOtter
Copy link
Member

I would suggest to just accept the current status of the pull request and merge it.

@tobolar
Copy link
Contributor

tobolar commented May 16, 2023

I would suggest to just accept the current status of the pull request and merge it.

Ok, I will fix the remaining models and we can merge then.

@TManikantan TManikantan requested a review from tobolar May 16, 2023 11:09
@TManikantan
Copy link
Contributor

thankyou @tobolar . I guess its you who have to again review it. @tobolar @hubertus65 would you please approve the PR?

@MartinOtter
Copy link
Member

Its not clear to me why "this commit cannot be built". Can someone help?

Copy link
Member

@hubertus65 hubertus65 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@hubertus65
Copy link
Member

It looks like merging is still blocked since github doesn't know that Dag is with Dassault and that there is a signed cla. I think that by now all actual code changes have been by @tobolar, so that it should be possible to back out Dag's commits and retry. Not sure if anybody here can override this issue. @MartinOtter, any chance of moving this forward?

@tobolar
Copy link
Contributor

tobolar commented May 19, 2023

I think that by now all actual code changes have been by @tobolar,...

Unfortunately, this doesn't apply. I've touched far less files then the initial commits of @DagBruck. No idea how to proceed.

@DagBruck
Copy link
Contributor Author

It looks like merging is still blocked since github doesn't know that Dag is with Dassault and that there is a signed cla. I think that by now all actual code changes have been by @tobolar, so that it should be possible to back out Dag's commits and retry. Not sure if anybody here can override this issue. @MartinOtter, any chance of moving this forward?

This is confusing but to me the fastest way forward was for me to sign the CLA electronically, even though it out of principle should not be needed. So far I have not seen any change in the status of this pull request.

@MartinOtter MartinOtter merged commit 6212997 into maint/4.0.x May 24, 2023
1 of 3 checks passed
@MartinOtter MartinOtter deleted the DagBruck_01 branch May 24, 2023 13:21
@TManikantan TManikantan modified the milestones: MSL4.1.0, MSL4.0.1 May 25, 2023
TManikantan added a commit that referenced this pull request Jun 1, 2023
* Resolved issue #4072 (for Modelica/Electrical)

* Resolved issue #4072 (for rest of Modelica)

* Squash doubled point {0,-50.5}

* Delete doubled connection's coordinate

* Delete doubled connection's coordinate

* Delete doubled connection's coordinate

* Delete doubled connection's coordinate

* Delete doubled connection's coordinate

* Delete doubled connection's coordinate

* Delete doubled connection's coordinate

* Fix near-zero coordinate in connection's annotation

* Merge branch 'DagBruck_01' of https://github.com/modelica/ModelicaStandardLibrary into DagBruck_01

---------

Co-authored-by: Dag Brück <dag.brueck@3ds.com>
Co-authored-by: tobolar <tobolar@users.noreply.github.com>
Co-authored-by: tobolar <jakub.tobolar@dlr.de>
beutlich added a commit to beutlich/ModelicaStandardLibrary that referenced this pull request Jun 2, 2023
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.

Near-zero coordinates in graphical annotations
7 participants