#2227 Move Transform functionality into Rewrite #2252

jameswinkler · 2021-01-21T00:20:22Z

....In preparation for the deletion of transform in its entirety.

Re-order commands alphabetically. Delete unused constructor.

…rrect unit tests

eddynaka · 2021-01-21T00:34:48Z

src/Sarif.Multitool/Program.cs

                (MergeOptions mergeOptions) => new MergeCommand().Run(mergeOptions),
                (PageOptions pageOptions) => new PageCommand().Run(pageOptions),
                (QueryOptions queryOptions) => new QueryCommand().Run(queryOptions),
                (RebaseUriOptions rebaseOptions) => new RebaseUriCommand().Run(rebaseOptions),
+                (ResultMatchingOptions baselineOptions) => new ResultMatchingCommand().Run(baselineOptions),


ResultMatchingCommand [](start = 63, length = 21)

I didn't set in the alphabetical order because the resultmatching is equal match-results-foward command. #Closed

Would it be worthwhile to rename the file/class to something like "MatchResultOptions" for consistency?

In reply to: 561433654 [](ancestors = 561433654)

Let's do it in another pr, separating what we are doing right now :)

In reply to: 561480791 [](ancestors = 561480791,561433654)

Done.

In reply to: 564739449 [](ancestors = 564739449,561480791,561433654)

…//github.com/microsoft/sarif-sdk into users/v-jwinkler/#2227_MergeTransformRewrite

…handle any version except 2.

…//github.com/microsoft/sarif-sdk into users/v-jwinkler/#2227_MergeTransformRewrite

…ersion 1

…g to V2 first always

eddynaka · 2021-01-26T18:31:25Z

src/Sarif.Driver/Sdk/CommandBase.cs

@@ -18,11 +18,9 @@ public abstract class CommandBase

        protected IFileSystem FileSystem { get; set; }

-        public CommandBase(IFileSystem fileSystem = null)


public CommandBase(IFileSystem fileSystem = null) [](start = 8, length = 49)

don't remove this. For now, this is the way we initialize the FileSytem. If you check, that is used in MultifileCommandBase, calling the FileSystem directly. #Closed

Fixed.

In reply to: 564738564 [](ancestors = 564738564)

eddynaka · 2021-01-26T18:34:45Z

src/Sarif.Multitool.Library/RewriteCommand.cs

+            return null;
+        }
+
+        private void TransformFileToVersionTwo(SingleFileOptionsBase options, string inputVersion, string outputFilePath)


void [](start = 16, length = 4)

can you return SarifLog instead of void?
in the if, you could return visitor.SarifLog

remove the else and return JsonConvert.... to SarifLog.
With this, we won't need to save in disk #Closed

i would change from options to the sarif string to facilitate, since we won't need to use the options

In reply to: 564740510 [](ancestors = 564740510)

Done.

In reply to: 564742595 [](ancestors = 564742595,564740510)

lgtm-com · 2021-01-26T18:40:40Z

This pull request fixes 1 alert when merging 79ca0bc into d7afbe6 - view on LGTM.com

fixed alerts:

1 for Useless assignment to local variable

eddynaka · 2021-01-26T18:52:32Z

src/Test.UnitTests.Sarif.Multitool.Library/ApplyPolicyCommandTests.cs

@@ -43,7 +43,8 @@ private SarifLog ExecuteTest(string path)
            {
                InputFilePath = path,
                OutputFilePath = path,
-                Force = true
+                Force = true,
+                SarifOutputVersion = SarifVersion.Current


SarifOutputVersion = SarifVersion.Current [](start = 16, length = 41)

should we add this as default value in the options itself? #Closed

Done.

In reply to: 564751819 [](ancestors = 564751819)

eddynaka · 2021-01-26T18:54:57Z

src/Sarif/Writers/ConsoleLogger.cs

@@ -11,6 +11,8 @@ namespace Microsoft.CodeAnalysis.Sarif.Writers
 {
    public class ConsoleLogger : BaseLogger, IAnalysisLogger
    {
+        //  TODO:  We directly instantiate this logger in two classes, creating 
+        //  unamanged dependencies.  Fix this pattern with dependency injection or a factory.


I don't think we have to worry in this. Since the lifetime of the use is a command, the unmanaged resource will go away when it finishes. #ByDesign

As discussed, created an issue for this.

In reply to: 564753259 [](ancestors = 564753259)

lgtm-com · 2021-01-26T19:04:34Z

This pull request fixes 1 alert when merging d537617 into d7afbe6 - view on LGTM.com

fixed alerts:

1 for Useless assignment to local variable

…ate writes while transforming

lgtm-com · 2021-01-26T20:09:09Z

This pull request fixes 1 alert when merging 405c18d into d7afbe6 - view on LGTM.com

fixed alerts:

1 for Useless assignment to local variable

lgtm-com · 2021-01-26T20:23:30Z

This pull request fixes 1 alert when merging 74c7c74 into d7afbe6 - view on LGTM.com

fixed alerts:

1 for Useless assignment to local variable

lgtm-com · 2021-01-26T21:37:49Z

This pull request fixes 1 alert when merging 263e636 into d7afbe6 - view on LGTM.com

fixed alerts:

1 for Useless assignment to local variable #Closed

lgtm-com · 2021-01-26T21:55:43Z

This pull request fixes 1 alert when merging 4066d45 into d7afbe6 - view on LGTM.com

fixed alerts:

1 for Useless assignment to local variable #Closed

lgtm-com · 2021-01-26T22:17:19Z

This pull request fixes 1 alert when merging f6efc3e into d7afbe6 - view on LGTM.com

fixed alerts:

1 for Useless assignment to local variable #Closed

eddynaka · 2021-01-26T22:32:17Z

src/ReleaseHistory.md

@@ -1,5 +1,7 @@
 # SARIF Package Release History (SDK, Driver, Converters, and Multitool)

+* BREAKING: Move "transform" functionality into "rewrite" and delete redundant "transform" command


" [](start = 17, length = 1)

change from " to `.
Add the pull request number and link.
Add period in the end #Closed

Done.

In reply to: 564879607 [](ancestors = 564879607)

eddynaka

lgtm-com · 2021-01-26T23:12:47Z

This pull request fixes 1 alert when merging 01d26b9 into d7afbe6 - view on LGTM.com

fixed alerts:

1 for Useless assignment to local variable

lgtm-com · 2021-01-28T13:11:55Z

This pull request fixes 1 alert when merging 8bf66a5 into 2dd2854 - view on LGTM.com

fixed alerts:

1 for Useless assignment to local variable

jameswinkler added 2 commits January 20, 2021 13:38

Move some code from TransformCommand to DriverExtensionMethods and co…

47e9796

…rrect unit tests

Refatoring, add a bunch of functionality to RewriteCommand

ac7ea6e

eddynaka reviewed Jan 21, 2021

View reviewed changes

eddynaka and others added 11 commits January 21, 2021 15:15

Merge branch 'main' into users/v-jwinkler/#2227_MergeTransformRewrite

ff37584

Add some comments, new method "TransformationNecessary"

5050c7f

Merge branch 'users/v-jwinkler/#2227_MergeTransformRewrite' of https:…

9bea969

…//github.com/microsoft/sarif-sdk into users/v-jwinkler/#2227_MergeTransformRewrite

Merge branch 'main' into users/v-jwinkler/#2227_MergeTransformRewrite

cc56480

Checkin preliminary changes, 4 unit tests fail because Rewrite can't …

1763489

…handle any version except 2.

Merge branch 'users/v-jwinkler/#2227_MergeTransformRewrite' of https:…

8067797

…//github.com/microsoft/sarif-sdk into users/v-jwinkler/#2227_MergeTransformRewrite

Merge main -> #2227_MergeTranformRewrite

e06c072

Transform to version 2 first, rewrite, then if needed, transform to v…

17af1e5

…ersion 1

Address failing unit tests, refactor per discussions. Now transformin…

8b700de

…g to V2 first always

Delete transform command entirely

188db02

One last comment

79ca0bc

jameswinkler marked this pull request as ready for review January 26, 2021 18:28

jameswinkler requested a review from michaelcfanning as a code owner January 26, 2021 18:28

eddynaka reviewed Jan 26, 2021

View reviewed changes

Restore constructor

d537617

eddynaka reviewed Jan 26, 2021

View reviewed changes

Default to current version, undo reordering of commands, and consolid…

405c18d

…ate writes while transforming

Fix formatting

74c7c74

jameswinkler added 2 commits January 26, 2021 13:23

Create issues for TODOs

263e636

Merge branch 'main' into users/v-jwinkler/#2227_MergeTransformRewrite

2d348c3

jameswinkler added 2 commits January 26, 2021 13:39

Add info to ReleaseHistory

8dc8f9b

Add one more issue for TODO

4066d45

Remove redundant assignments

f6efc3e

eddynaka reviewed Jan 26, 2021

View reviewed changes

eddynaka approved these changes Jan 26, 2021

View reviewed changes

jameswinkler and others added 2 commits January 26, 2021 14:50

correct release history

a06f8e4

Update ReleaseHistory.md

01d26b9

Merge branch 'main' into users/v-jwinkler/#2227_MergeTransformRewrite

8bf66a5

michaelcfanning merged commit 689b73d into main Jan 28, 2021

michaelcfanning deleted the users/v-jwinkler/#2227_MergeTransformRewrite branch January 28, 2021 16:11

jameswinkler mentioned this pull request Jan 28, 2021

Transform (soon to be rewrite) command does not honor --minify #2256

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2227 Move Transform functionality into Rewrite #2252

#2227 Move Transform functionality into Rewrite #2252

jameswinkler commented Jan 21, 2021

eddynaka Jan 21, 2021 •

edited

jameswinkler Jan 21, 2021

eddynaka Jan 26, 2021

jameswinkler Jan 26, 2021

eddynaka Jan 26, 2021 •

edited

jameswinkler Jan 26, 2021

eddynaka Jan 26, 2021 •

edited

eddynaka Jan 26, 2021

jameswinkler Jan 26, 2021

lgtm-com bot commented Jan 26, 2021

eddynaka Jan 26, 2021 •

edited

jameswinkler Jan 26, 2021

eddynaka Jan 26, 2021 •

edited

jameswinkler Jan 26, 2021

lgtm-com bot commented Jan 26, 2021

lgtm-com bot commented Jan 26, 2021

lgtm-com bot commented Jan 26, 2021

lgtm-com bot commented Jan 26, 2021 •

edited by eddynaka

lgtm-com bot commented Jan 26, 2021 •

edited by eddynaka

lgtm-com bot commented Jan 26, 2021 •

edited by eddynaka

eddynaka Jan 26, 2021 •

edited

jameswinkler Jan 26, 2021

eddynaka left a comment

lgtm-com bot commented Jan 26, 2021

lgtm-com bot commented Jan 28, 2021

		@@ -18,11 +18,9 @@ public abstract class CommandBase

		protected IFileSystem FileSystem { get; set; }

		public CommandBase(IFileSystem fileSystem = null)

		@@ -1,5 +1,7 @@
		# SARIF Package Release History (SDK, Driver, Converters, and Multitool)

		* BREAKING: Move "transform" functionality into "rewrite" and delete redundant "transform" command

#2227 Move Transform functionality into Rewrite #2252

#2227 Move Transform functionality into Rewrite #2252

Conversation

jameswinkler commented Jan 21, 2021

eddynaka Jan 21, 2021 • edited

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

eddynaka Jan 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Jan 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Jan 26, 2021

eddynaka Jan 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Jan 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Jan 26, 2021

lgtm-com bot commented Jan 26, 2021

lgtm-com bot commented Jan 26, 2021

lgtm-com bot commented Jan 26, 2021 • edited by eddynaka

lgtm-com bot commented Jan 26, 2021 • edited by eddynaka

lgtm-com bot commented Jan 26, 2021 • edited by eddynaka

eddynaka Jan 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Jan 26, 2021

lgtm-com bot commented Jan 28, 2021

eddynaka Jan 21, 2021 •

edited

eddynaka Jan 26, 2021 •

edited

eddynaka Jan 26, 2021 •

edited

eddynaka Jan 26, 2021 •

edited

eddynaka Jan 26, 2021 •

edited

lgtm-com bot commented Jan 26, 2021 •

edited by eddynaka

lgtm-com bot commented Jan 26, 2021 •

edited by eddynaka

lgtm-com bot commented Jan 26, 2021 •

edited by eddynaka

eddynaka Jan 26, 2021 •

edited