DYN-1819 ExportExcel Write as String Option #11291

StudioLE · 2020-11-30T15:38:58Z

Purpose

Added an additional writeAsString parameter to the Data.ExportExcel node. This parameter defaults to false ensuring the existing behaviour is preserved. When set to true the NumberFormat for the written range is set to text, therefore preserving the string rather than letting Excel automatically determine the format.

Behaviour before:

Behaviour after writeAsString: false:

Behaviour after writeAsString: true:

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.
This PR modifies some build requirements and the readme is updated

Reviewers

@mjkkirschner @QilongTang @mmisol

FYIs

@Amoursol

mmisol · 2020-11-30T20:06:26Z

Hi @StudioLE . Could you please add a unit test for this?

Edit: For reference, here is the file where we have the unit tests related to Excel: https://github.com/DynamoDS/Dynamo/blob/master/test/Libraries/DynamoMSOfficeTests/ExcelTests.cs

mjkkirschner · 2020-11-30T20:40:13Z

I don't think these changes are allowed in terms of API compatability...
see
Adding a parameter with a default value.
https://stackoverflow.com/questions/1456785/a-definitive-guide-to-api-breaking-changes-in-net

see:
https://docs.microsoft.com/en-us/dotnet/core/compatibility/

❌ DISALLOWED: Adding, removing, or changing the order of parameters

while this might work pretty well for nodes - I don't think it will work for c# code that calls this code.

mmisol · 2020-11-30T20:52:38Z

@mjkkirschner Right, but this PR is doing what's described in the ticket. Could it be that we didn't consider this part of our API or disregarded the compatibility break? Copying @Amoursol

Amoursol · 2020-11-30T21:09:49Z

@mjkkirschner Right, but this PR is doing what's described in the ticket. Could it be that we didn't consider this part of our API or disregarded the compatibility break? Copying @Amoursol

I had indeed not been aware of the compatibility break - assumed that adding a new node port (As we have done in other cases?) would be acceptable and a non-breaking change if default values adhered to existing graph conditions.

@mjkkirschner - What kind of issues will arise from this in a non-node basis, assuming my above assertions are correct?

mjkkirschner · 2020-11-30T21:14:19Z

code which calls this node from c# will throw a MissingMethodException - so if a user has written another zero touch node or extension which references this will likely result in a failing node or crash from the extension.

What should likely be done is to:

obsolete these nodes.
Make sure they are hidden in the library (obsolete should do this AFAIK)
Create a new node with the new input.
Consider looking into using the [AlsoKnownAs] attribute - I can't remember the details of this attribute but it's made for migration/loading older graphs with new nodes.

mjkkirschner · 2020-11-30T21:24:29Z

it may make sense to use the migrations.xml file instead since theres already an existing one for dsoffice assembly:
https://github.com/DynamoDS/Dynamo/blob/master/src/Libraries/DSOffice/DSOffice.Migrations.xml

Either of these migration options will need manual and automated testing.

mmisol · 2020-12-08T15:55:52Z

@StudioLE Based on the comments made by @mjkkirschner , could we ask you to change the contribution to use a new node/function so that we can remain backwards compatible? Thank you

…xcel-As-String

SHKnudsen · 2020-12-08T18:14:05Z

Hi guys, im looking into getting this wrapped up, Sorry for the delay!

Im having some issues getting the migrations to work. I tried the following:

Create new node (ExportExcelWithStringParameter) <- name was just for testing btw..
Make a new migration:

  <priorNameHint>
    <oldName>DSOffice.Data.ExportExcel</oldName>
    <newName>DSOffice.Data.ExportExcelWithStringParameter</newName>
  </priorNameHint>

obsolete the old node so it dosent show in the library

Is there something im doing wrong here, it dosent really work at least. When i open a graph using the old ExportExcel node it dosent use the new ExportExcelWithStringParameter it just shows the old in a warning state as it is obsolete. For some reason copy and pasting the obsolete node will make a ExportExcelWithStringParameter node but renamed as ExportExcel.

mmisol · 2020-12-08T18:49:45Z

Hi @SHKnudsen . I'm not familiar with the migrations but maybe @mjkkirschner and @aparajit-pratap can chime in?

aparajit-pratap · 2020-12-08T19:14:25Z

@SHKnudsen could you also try including the list of parameters with both node names in the migrations.xml file since the new node also has an additional parameter. Refer to this for an example:

Dynamo/extern/ProtoGeometry/ProtoGeometry.Migrations.xml

Line 76 in b39ac2a

    
           <oldName>Autodesk.DesignScript.Geometry.Surface.ByLoft@Autodesk.DesignScript.Geometry.Curve[],Autodesk.DesignScript.Geometry.Curve</oldName>

mjkkirschner · 2020-12-09T16:23:38Z

@SHKnudsen @StudioLE - for now can you try the following:

Obsolete the existing node.
Add a new node with the extra parameter - make sure this node has a new name - do not use the same name.
Nevermind the migration for now.

SHKnudsen · 2020-12-09T16:41:15Z

@mjkkirschner done.

Called the node ExportToExcel for now, hard to find a different name than the original. Let me know if there are other suggestions for the name.

src/Libraries/DSOffice/Excel.cs

aparajit-pratap

@SHKnudsen thanks! You'll need to do one last thing - add an entry for the new node to the libraryItems.json file so that the remaining failing test passes. See: https://github.com/DynamoDS/Dynamo/wiki/Adding-or-Updating-Built-In-Library-Icons#step-5-update-testing-resources

This reverts commit 0ddd62d.

mmisol · 2020-12-10T13:09:33Z

src/Libraries/DSOffice/DSOfficeImages.resx

+        iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAABzenr0AAAABGdBTUEAALGPC/xhBQAAABl0RVh0U29m
+        dHdhcmUAQWRvYmUgSW1hZ2VSZWFkeXHJZTwAAABMSURBVFhH7dCxCQAgDETRTCuO4Gwup5YpzkKuMfAf
+        pPmFcAYA4GbMvvK57Zl6xGl4dr6u5XNbPWqF0+pRK5xWj1rhtHrUCqcBwKciNnx+8yHggVbjAAAAAElF
+        TkSuQmCC


Did this icon change?

I had to rename the icon. Guess thats whats triggering the change?

It could be some randomness in the resx file introduced by Visual Studio, but it is strange because this is another icon, not the one that was renamed.

Oh right, hmm that is strange. Not even sure what this icon is used for, there is only a Small version of it

mmisol · 2020-12-10T13:11:24Z

src/Libraries/DSOffice/DSOfficeImages.resx

+        xeDuk3//vu9nyGkkgJc832/yXdf8GTP0iWb3zoUWlxPLyEMb9wetSfDa9u+J/tm0YxIMrYh2w0I6YPIw
+        J/mbonv8D7zpnw8R+16TMAsWlk+Gvzl7yLn8Oxp6xHyahHljgWsS5o0FrkmYNxa4JmHeWOCahHljgWsS
+        5o0FrkmYNxZ4nASdnH1uLPRmEhT+vPBJwEchhBBCCCGEEEIIIYQQQgghxBSsrHwC4O+ISk6Usf4AAAAA
+        SUVORK5CYII=


The content for this one looks different too. Is that expected?

Yea not sure whats going on here, haven't touched this either

I checked and in Dynamo the icons of the node look ok, so this is harmless.

mmisol · 2020-12-10T13:18:20Z

src/Libraries/DSOffice/Excel.cs

        /// <returns name="data">Data written to the spreadsheet.</returns>
        /// <search>office,excel,spreadsheet</search>
        [IsVisibleInDynamoLibrary(false)]
-        public static object[][] WriteToFile(string filePath, string sheetName, int startRow, int startCol, object[][] data, bool overWrite = false)
+        public static object[][] WriteToFile(string filePath, string sheetName, int startRow, int startCol, object[][] data, bool overWrite = false, bool writeAsString = false)


This function is having a binary breaking change because optional parameters are a compile-time feature. Can we use an overload instead?

Sorry, hadnt realized this was public too, will fix that.

This new method doesn't need to be public. This logic can actually be folded into the new ExportToExcel node and you can remove this method entirely. The only reason there existed an old WriteToFile method was that it was originally a node, which was later renamed to ExportExcel.

So one thing before i make this change. Would you prefer to have one private method that WriteToFile and ExportToExcel uses? other option would be to duplicate the logic from WriteToFile into ExportToExcel with the additional parameter.

I think option 1 sounds good. I'd rather not duplicate the code if possible.

mmisol · 2020-12-10T13:24:01Z

src/Libraries/DSOffice/DSOfficeImages.resx

  </data>
-  <data name="DSOffice.Data.ExportExcel.Small" type="System.Drawing.Bitmap, System.Drawing" mimetype="application/x-microsoft.net.object.bytearray.base64">
+  <data name="DSOffice.Data.ExportToExcel.Small" type="System.Drawing.Bitmap, System.Drawing" mimetype="application/x-microsoft.net.object.bytearray.base64">


Since we are not doing the migration yet, I think we should keep the icon for the old node too, rather than replace it.

Okay, but we still use the same icon for both right?

Yes, that should be fine.

mmisol

Looks good to me

mmisol · 2020-12-10T16:51:17Z

src/Libraries/DSOffice/DSOfficeImages.resx

+        xeDuk3//vu9nyGkkgJc832/yXdf8GTP0iWb3zoUWlxPLyEMb9wetSfDa9u+J/tm0YxIMrYh2w0I6YPIw
+        J/mbonv8D7zpnw8R+16TMAsWlk+Gvzl7yLn8Oxp6xHyahHljgWsS5o0FrkmYNxa4JmHeWOCahHljgWsS
+        5o0FrkmYNxZ4nASdnH1uLPRmEhT+vPBJwEchhBBCCCGEEEIIIYQQQgghxBSsrHwC4O+ISk6Usf4AAAAA
+        SUVORK5CYII=


I checked and in Dynamo the icons of the node look ok, so this is harmless.

aparajit-pratap

One last comment about the redundancy of the new WriteToFile method.

mmisol · 2020-12-11T15:50:59Z

@aparajit-pratap Could I ask you take one final look here? If you are ok with the last change I think we are good to merge.

aparajit-pratap · 2020-12-11T15:54:36Z

src/Libraries/DSOffice/Excel.cs

@@ -864,7 +845,7 @@ public static object[][] ImportExcel(FileInfo file, string sheetName, bool readA



Is there a line white space here? Can we delete this?

aparajit-pratap · 2020-12-11T17:08:17Z

@mmisol are we going to add any tests or have Sylvester add tests?

mmisol · 2020-12-11T17:11:18Z

Good point @aparajit-pratap . I didn't realize we didn't have them. Could you take care of adding one @SHKnudsen ?

SHKnudsen · 2020-12-11T17:12:31Z

Yes sorry guys, actually thought there was tests on this one. Ill have a look at it now.

SHKnudsen · 2020-12-11T18:55:49Z

Added a simple test that checks if the exported value is converted to string when the writeAsString is set to true.

mmisol · 2020-12-11T19:10:38Z

Thanks @SHKnudsen . I'll wait for self serve and merge.

QilongTang · 2020-12-17T18:47:45Z

test/core/excel/ExportToExcelAsString.dyn

+  "Nodes": [
+    {
+      "ConcreteType": "CoreNodeModels.Input.Filename, CoreNodeModels",
+      "HintPath": "C:\\Users\\SylvesterKnudsen\\Documents\\GitHub\\StudioLEDynamo\\test\\core\\excel\\WriteFile.xlsx",


I think we missed this during the PR review.. Not sure how self CI passed with this...

@QilongTang Now that you mention it, I think @alfredo-pozo said ExcelTests are not run in self-serve CI. This is my bad, sorry for forgetting.

@QilongTang @mmisol - the weird part is why did we not get a master-15 job until today that contained this code?

ExportExcel write as string option

14b01eb

StudioLE changed the title ~~ExportExcel write as string option~~ DYN-1819 ExportExcel Write as String Option Nov 30, 2020

mmisol self-requested a review November 30, 2020 20:00

Merge remote-tracking branch 'upstream/master' into DYN-1819-Export-E…

245be7c

…xcel-As-String

add new excel node with writeAsString argument

604caea

aparajit-pratap reviewed Dec 9, 2020

View reviewed changes

src/Libraries/DSOffice/Excel.cs Show resolved Hide resolved

aparajit-pratap reviewed Dec 9, 2020

View reviewed changes

src/Libraries/DSOffice/Excel.cs Outdated Show resolved Hide resolved

update name

0ddd62d

aparajit-pratap reviewed Dec 9, 2020

View reviewed changes

SHKnudsen added 2 commits December 10, 2020 08:14

Revert "update name"

68790fb

This reverts commit 0ddd62d.

Update libraryItems.json

9a8736c

mmisol reviewed Dec 10, 2020

View reviewed changes

overload WriteToFile + add icons back for old excel node

6125a9e

mmisol approved these changes Dec 10, 2020

View reviewed changes

aparajit-pratap reviewed Dec 10, 2020

View reviewed changes

make internal WriteData method

d75c4f8

aparajit-pratap approved these changes Dec 11, 2020

View reviewed changes

remove line

34e8498

add test

995712a

mmisol merged commit 5b9643f into DynamoDS:master Dec 11, 2020

QilongTang reviewed Dec 17, 2020

View reviewed changes

Amoursol mentioned this pull request Jan 21, 2021

Excel Export AsString DynamoDS/DynamoWishlist#13

Closed

		@@ -864,7 +845,7 @@ public static object[][] ImportExcel(FileInfo file, string sheetName, bool readA

DYN-1819 ExportExcel Write as String Option #11291

DYN-1819 ExportExcel Write as String Option #11291

Conversation

StudioLE commented Nov 30, 2020

Purpose

Declarations

Reviewers

FYIs

mmisol commented Nov 30, 2020 • edited Loading

mjkkirschner commented Nov 30, 2020 • edited Loading

mmisol commented Nov 30, 2020

Amoursol commented Nov 30, 2020

mjkkirschner commented Nov 30, 2020

mjkkirschner commented Nov 30, 2020 • edited Loading

mmisol commented Dec 8, 2020 • edited Loading

SHKnudsen commented Dec 8, 2020

mmisol commented Dec 8, 2020

aparajit-pratap commented Dec 8, 2020

mjkkirschner commented Dec 9, 2020

SHKnudsen commented Dec 9, 2020

aparajit-pratap left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aparajit-pratap Dec 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmisol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aparajit-pratap left a comment

Choose a reason for hiding this comment

mmisol commented Dec 11, 2020

Choose a reason for hiding this comment

aparajit-pratap commented Dec 11, 2020

mmisol commented Dec 11, 2020

SHKnudsen commented Dec 11, 2020

SHKnudsen commented Dec 11, 2020

mmisol commented Dec 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmisol commented Nov 30, 2020 •

edited

Loading

mjkkirschner commented Nov 30, 2020 •

edited

Loading

mjkkirschner commented Nov 30, 2020 •

edited

Loading

mmisol commented Dec 8, 2020 •

edited

Loading

aparajit-pratap left a comment •

edited

Loading

aparajit-pratap Dec 10, 2020 •

edited

Loading