Compliance Update and Enable Update Panels #310

rwemay · 2020-02-03T14:34:36Z

NOTE: Depends on

Issues addressed by this PR

Closes #308
Closes #301
Closes #250
Closes #184
Closes #249
Closes #319
Closes #318
Closes #321
Closes #322

Test files

Test files and models

Changelog

Additional comments

rwemay · 2020-02-04T21:58:46Z

@IsakNaslundBh , @kThorsager , @alelom this is now getting there. I've fixed a few things related to panel update, specifically the support/releases for the edges, which were not properly implemented before. One thing to check before fully test/merge:

I've added a call to Update methods for panel edge supports and releases - so if the label already exists in Robot, the properties of the label will be updated (supported/released directions). I'm not sure if this is the intention of the update methods - to update dependent properties, but if it is I think we need a different solution here. The way I've currently written, this will effectively be re-writing the same properties many times.
Assuming we should be updating dependent properties, I need to implement something for other properties - e.g. panel thickness, if the name is the same.
I'm not sure how the adapter is handling checking uniqueness of properties for similar named objects - so for example, if someone pushes lots of bars, with releases, and the releases have the same name, but different release directions, presumably all bars will have releases based on the first one pushed?

More questions than answers :) I'm off tomorrow, so catch up Thursday/later this week. Either way not one to rush through given extent of change necessary.

IsakNaslundBh

Really nice to get this in!

Some comments on some things that will need updating. Have just read the code, no tests yet.

IsakNaslundBh · 2020-02-05T08:52:51Z

Robot_Adapter/CRUD/Update/Panels.cs

+        /****           Protected Methods               ****/
+        /***************************************************/
+
+        protected override bool UpdateOnly<T>(IEnumerable<T> panels, string tag = "", ActionConfig actionConfig = null)


Would not override this method, but just change it do an Update method that then can be dynamically called by the IUpdate mehtod. (similar principle as what we do with the Create)

By overriding this method UpdateOnly will only work for panels, but no other objects.

Sorry missed that one... was still getting to grips with new adapter. Updated now and reran just to check - all ok. Have you seen my comments in message above? Line 165 and 193 in this cs file are relevant. I'm not sure it's 'correct' to be calling update on properties of dependent objects, and if it is, I don't think this approach is sustainable (and will have a big performance hit as it scales).

Robot_Engine/Convert/ToRobot/Release.cs

Update fixed

IsakNaslundBh

Testing if rebase possible

Dismissing test review

rwemay · 2020-02-14T16:04:10Z

@IsakNaslundBh , I've tidied up this PR and network graph and I think/hope is ready for testing. Maybe one for us to talk/work though? Essentially final change is removing the auto-generated new properties when a named property (support/release) already exists, but returning a warning if we modify the named property in the model. Eventually I'd like to return 'one' warning to 'check push report' down the line - once we've built out the push report (returned as part of the 'success').

FraserGreenroyd · 2020-02-15T09:57:26Z

/azp run Robot_Toolkit.CheckCompliance

azure-pipelines · 2020-02-15T09:57:34Z

Azure Pipelines successfully started running 1 pipeline(s).

FraserGreenroyd

I've had a look at the compliance report on this and am requesting changes because there are a few files which do not appear to conform to current guidelines. Please review the report on the details for more information or reach out if anything is unclear 😄

Also, this PR needs an appropriate label (bug, feature, compliance, etc.) in addition to the size for the change log please.

rwemay · 2020-02-16T15:43:39Z

Thanks @FraserGreenroyd , we were going to hold off the compliance updates for this/do it gradually. I see the tests are failing because we done a first pass (move cs files). Will see if I can try and add compliance changes too.

@IsakNaslundBh I've found another issue with the adapter update cycle, so have added do not merge label again... sorry. Hopefully will fix tomorrow.

azure-pipelines · 2020-02-19T17:00:48Z

Azure Pipelines successfully started running 1 pipeline(s).

@IsakNaslundBh

thanks @IsakNaslundBh !

Robot_Adapter/CRUD/Update/Elements/Panels.cs

rwemay · 2020-02-20T17:19:51Z

/azp run Robot_Toolkit.CheckCompliance

azure-pipelines · 2020-02-20T17:20:00Z

Azure Pipelines successfully started running 1 pipeline(s).

rwemay · 2020-02-21T15:06:43Z

/azp run Robot_Toolkit.CheckCompliance

azure-pipelines · 2020-02-21T15:06:53Z

Azure Pipelines successfully started running 1 pipeline(s).

FraserGreenroyd

I'm happy with this from a compliance perspective, thanks @rwemay - will allow @IsakNaslundBh the final approval from a functionality perspective 😄

Am happy

IsakNaslundBh

Code looks a lot better and all test runs works as good or better than before.

Would say lets get this merged, and if any bugs pop up, lets solve them later (do not think there should be any major induced by this PR)

IsakNaslundBh · 2020-02-21T15:46:59Z

/azp run Robot_Toolkit.CheckCompliance

azure-pipelines · 2020-02-21T15:47:09Z

Azure Pipelines successfully started running 1 pipeline(s).

IsakNaslundBh · 2020-02-21T15:47:22Z

/azp run Robot_Toolkit.CheckInstaller

azure-pipelines · 2020-02-21T15:47:31Z

Azure Pipelines successfully started running 1 pipeline(s).

rwemay added the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Feb 4, 2020

IsakNaslundBh previously requested changes Feb 5, 2020

View reviewed changes

IsakNaslundBh previously approved these changes Feb 7, 2020

View reviewed changes

IsakNaslundBh self-requested a review February 7, 2020 08:37

rwemay added 8 commits February 14, 2020 11:50

Update panel geometry working prototype

56545ce

UpdateOnly methods for Panels

81887f1

Allow update edge support properties

d4b9a25

Fix Linear Release assignments for Panels

154109a

Updates to support and constraint update methods

c595dcf

update Update method call

bc1adcd

Tidy up of private helpers, and update of property create methods

d3b7d04

Append to label names

1b8a57c

rwemay force-pushed the Robot_Toolkit-#Issue308-UpdateOnlyMethods branch from 1c2c44c to 1b8a57c Compare February 14, 2020 11:52

Add warnings when properties are updated

52eb510

rwemay added size:M Measured in hours and removed status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge labels Feb 14, 2020

rwemay self-assigned this Feb 14, 2020

Constraint update and create methods fixed. Thickness properties ToDo

724f114

FraserGreenroyd previously requested changes Feb 15, 2020

View reviewed changes

rwemay added status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge type:feature New capability or enhancement labels Feb 16, 2020

rwemay added 2 commits February 19, 2020 17:13

Minor updates following Azure compliance checks

476e34e

Moving comparer classes to meet CI compliance

f0f282b

thanks @IsakNaslundBh !

IsakNaslundBh reviewed Feb 20, 2020

View reviewed changes

Robot_Adapter/CRUD/Update/Elements/Panels.cs Outdated Show resolved Hide resolved

Panel update method to add null checks, and ensure openings are hosted

bcc9cc7

rwemay and others added 6 commits February 20, 2020 17:35

Added fix/checks to read panels/materials

1d0723c

Panel geometry and opening fixes for read and create methods

807280e

Fix Arc convert to Robot method

56f7ccb

Remove find openings method

4739646

Fix making sure cladding becomes cladding

e89fafa

making sure two-way get AllSides for loading panel properties

d8e3f90

rwemay requested a review from FraserGreenroyd February 21, 2020 14:50

rwemay removed the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Feb 21, 2020

FraserGreenroyd reviewed Feb 21, 2020

View reviewed changes

Fix compliance

28f0095

IsakNaslundBh approved these changes Feb 21, 2020

View reviewed changes

IsakNaslundBh merged commit a924350 into master Feb 21, 2020

IsakNaslundBh deleted the Robot_Toolkit-#Issue308-UpdateOnlyMethods branch February 21, 2020 15:51

IsakNaslundBh mentioned this pull request Mar 2, 2020

Robot_Toolkit: Clean up section property management #286

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compliance Update and Enable Update Panels #310

Compliance Update and Enable Update Panels #310

rwemay commented Feb 3, 2020 •

edited

Loading

rwemay commented Feb 4, 2020

IsakNaslundBh left a comment

IsakNaslundBh Feb 5, 2020

rwemay Feb 5, 2020

IsakNaslundBh left a comment

rwemay commented Feb 14, 2020 •

edited

Loading

FraserGreenroyd commented Feb 15, 2020

azure-pipelines bot commented Feb 15, 2020

FraserGreenroyd left a comment •

edited

Loading

rwemay commented Feb 16, 2020

azure-pipelines bot commented Feb 19, 2020

rwemay commented Feb 20, 2020

azure-pipelines bot commented Feb 20, 2020

rwemay commented Feb 21, 2020

azure-pipelines bot commented Feb 21, 2020

FraserGreenroyd left a comment

IsakNaslundBh left a comment

IsakNaslundBh commented Feb 21, 2020

azure-pipelines bot commented Feb 21, 2020

IsakNaslundBh commented Feb 21, 2020

azure-pipelines bot commented Feb 21, 2020

Compliance Update and Enable Update Panels #310

Compliance Update and Enable Update Panels #310

Conversation

rwemay commented Feb 3, 2020 • edited Loading

NOTE: Depends on

Issues addressed by this PR

Test files

Changelog

Additional comments

rwemay commented Feb 4, 2020

IsakNaslundBh left a comment

Choose a reason for hiding this comment

IsakNaslundBh Feb 5, 2020

Choose a reason for hiding this comment

rwemay Feb 5, 2020

Choose a reason for hiding this comment

IsakNaslundBh left a comment

Choose a reason for hiding this comment

rwemay commented Feb 14, 2020 • edited Loading

FraserGreenroyd commented Feb 15, 2020

azure-pipelines bot commented Feb 15, 2020

FraserGreenroyd left a comment • edited Loading

Choose a reason for hiding this comment

rwemay commented Feb 16, 2020

azure-pipelines bot commented Feb 19, 2020

rwemay commented Feb 20, 2020

azure-pipelines bot commented Feb 20, 2020

rwemay commented Feb 21, 2020

azure-pipelines bot commented Feb 21, 2020

FraserGreenroyd left a comment

Choose a reason for hiding this comment

IsakNaslundBh left a comment

Choose a reason for hiding this comment

IsakNaslundBh commented Feb 21, 2020

azure-pipelines bot commented Feb 21, 2020

IsakNaslundBh commented Feb 21, 2020

azure-pipelines bot commented Feb 21, 2020

rwemay commented Feb 3, 2020 •

edited

Loading

rwemay commented Feb 14, 2020 •

edited

Loading

FraserGreenroyd left a comment •

edited

Loading