format-379: Add `--report` command line argument to export json format report to given directory #495

ghost · 2020-01-21T04:10:38Z

PR for this task. I diverged a tad from the original ask (generate a report when the --dry-run is provided) and decided that this would probably be useful in both normal and --dry-run modes. Also allowed the user to provide the output directory for the report, which I felt would be handy too (e.g. publishing it to a report viewer in Jenkins).

…json report of which files have/would have been formatted

dnfclas · 2020-01-21T04:10:51Z

All CLA requirements met.

src/dotnet-format.csproj

tests/Formatters/AbstractFormatterTests.cs

src/Formatters/ICodeFormatter.cs

ghost · 2020-01-21T04:54:50Z

Below is an example output of this change. Let me know if you'd prefer this be converted to camelCase.

[
  {
    "DocumentId": {
      "ProjectId": {
        "Id": "18cf4766-380a-4dc6-ab56-bc1e3b7625a8"
      },
      "Id": "81d1c87b-2187-4a7c-905d-9c413b17f0af"
    },
    "FileName": "CodeFormatter.cs",
    "FilePath": "C:\\projects\\format\\src\\CodeFormatter.cs",
    "FileChanges": [
      {
        "LineNumber": 291,
        "CharNumber": 30,
        "FormatDescription": "Fix whitespace formatting."
      }
    ]
  }
]

jmarolf · 2020-01-21T15:51:44Z

@JoeRobich ptal

src/CodeFormatter.cs

jmarolf · 2020-01-21T16:04:38Z

@jpshrader looks like a fantastic start! Thanks for the controbution! I left a few comments and I want @JoeRobich to take a look before merging

src/FileChange.cs

src/FormattedFile.cs

JoeRobich · 2020-01-21T17:48:02Z

src/Program.cs

@@ -36,13 +36,14 @@ private static async Task<int> Main(string[] args)
                .AddOption(new Option(new[] { "--dry-run" }, Resources.Format_files_but_do_not_save_changes_to_disk, new Argument<bool>()))
                .AddOption(new Option(new[] { "--check" }, Resources.Terminate_with_a_non_zero_exit_code_if_any_files_were_formatted, new Argument<bool>()))
                .AddOption(new Option(new[] { "--files" }, Resources.A_comma_separated_list_of_relative_file_paths_to_format_All_files_are_formatted_if_empty, new Argument<string>(() => null)))
+                .AddOption(new Option(new[] { "--report" }, Resources.Accepts_a_file_path_which_if_provided_will_produce_a_format_report_json_file_in_the_given_directory, new Argument<string>(() => null)))


If we made the default value ".", then if just the option is provided it would drop the report in the current folder.

I think there may be difficulty distinguishing between providing an empty argument vs not providing the option at all (unless I'm missing something). Changing the default argument to "." will set 'reportPath` even if you don't provide the argument at all.

Ah, good point.

src/CodeFormatter.cs

JoeRobich · 2020-01-21T17:53:15Z

@jpshrader Thanks for working on this. It is a good start to providing more report options such as patch file support.

1. Add unit tests for Formatted Files behaviour 2. Use System.Text.Json 3. Path.Combine/StringBuilder and pathing options 4. Pass ref to list instead of returning tuple

jmarolf · 2020-02-01T22:48:41Z

@jpshrader looks good to me! If @JoeRobich signs off I'll merge it

JoeRobich

Thanks so much for this. Just requesting a few changes before we merge it in. =)

JoeRobich · 2020-02-03T17:50:53Z

src/Program.cs

@@ -36,13 +36,14 @@ private static async Task<int> Main(string[] args)
                .AddOption(new Option(new[] { "--dry-run" }, Resources.Format_files_but_do_not_save_changes_to_disk, new Argument<bool>()))
                .AddOption(new Option(new[] { "--check" }, Resources.Terminate_with_a_non_zero_exit_code_if_any_files_were_formatted, new Argument<bool>()))
                .AddOption(new Option(new[] { "--files" }, Resources.A_comma_separated_list_of_relative_file_paths_to_format_All_files_are_formatted_if_empty, new Argument<string>(() => null)))
+                .AddOption(new Option(new[] { "--report" }, Resources.Accepts_a_file_path_which_if_provided_will_produce_a_format_report_json_file_in_the_given_directory, new Argument<string>(() => null)))


Ah, good point.

tests/Formatters/FormattedFilesTests.cs

src/Resources.resx

src/Formatters/DocumentFormatter.cs

…field

JoeRobich

LGTM!

@jmarolf do you have any final thoughts?

jmarolf · 2020-02-05T01:55:33Z

@JoeRobich looks good to merge to me!

ghost · 2020-02-05T01:59:35Z

Awesome, thanks @JoeRobich / @jmarolf ! Appreciate the reviews!

format-379-report: Add --report command line argument to produce a …

6005b1b

…json report of which files have/would have been formatted

ghost commented Jan 21, 2020

View reviewed changes

src/dotnet-format.csproj Outdated Show resolved Hide resolved

ghost commented Jan 21, 2020

View reviewed changes

tests/Formatters/AbstractFormatterTests.cs Outdated Show resolved Hide resolved

ghost commented Jan 21, 2020

View reviewed changes

src/Formatters/ICodeFormatter.cs Outdated Show resolved Hide resolved

ghost mentioned this pull request Jan 21, 2020

Dry run output format options #379

Closed

ghost changed the title ~~format-379-report: Add --report command line argument to export json format report to given directory~~ format-379: Add --report command line argument to export json format report to given directory Jan 21, 2020

jmarolf requested a review from JoeRobich January 21, 2020 15:51

jmarolf reviewed Jan 21, 2020

View reviewed changes

src/CodeFormatter.cs Outdated Show resolved Hide resolved

jmarolf reviewed Jan 21, 2020

View reviewed changes

src/CodeFormatter.cs Outdated Show resolved Hide resolved

jmarolf reviewed Jan 21, 2020

View reviewed changes

src/CodeFormatter.cs Outdated Show resolved Hide resolved

jmarolf reviewed Jan 21, 2020

View reviewed changes

src/CodeFormatter.cs Outdated Show resolved Hide resolved

JoeRobich reviewed Jan 21, 2020

View reviewed changes

PR feedback

eaf04f2

1. Add unit tests for Formatted Files behaviour 2. Use System.Text.Json 3. Path.Combine/StringBuilder and pathing options 4. Pass ref to list instead of returning tuple

ghost requested review from jmarolf and JoeRobich February 1, 2020 18:26

JoeRobich reviewed Feb 3, 2020

View reviewed changes

Format-379: param ordering, and break out test variable into private …

58c2316

…field

JoeRobich approved these changes Feb 5, 2020

View reviewed changes

JoeRobich merged commit 22cf1af into dotnet:master Feb 5, 2020

JoeRobich mentioned this pull request Feb 13, 2020

Update version to 3.3 #539

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

format-379: Add `--report` command line argument to export json format report to given directory #495

format-379: Add `--report` command line argument to export json format report to given directory #495

ghost commented Jan 21, 2020 •

edited by ghost

Loading

dnfclas commented Jan 21, 2020 •

edited

Loading

ghost commented Jan 21, 2020

jmarolf commented Jan 21, 2020

jmarolf commented Jan 21, 2020

JoeRobich Jan 21, 2020

ghost Jan 23, 2020

JoeRobich Feb 3, 2020

JoeRobich commented Jan 21, 2020

jmarolf commented Feb 1, 2020

JoeRobich left a comment

JoeRobich Feb 3, 2020

JoeRobich left a comment

jmarolf commented Feb 5, 2020

ghost commented Feb 5, 2020

format-379: Add --report command line argument to export json format report to given directory #495

format-379: Add --report command line argument to export json format report to given directory #495

Conversation

ghost commented Jan 21, 2020 • edited by ghost Loading

dnfclas commented Jan 21, 2020 • edited Loading

ghost commented Jan 21, 2020

jmarolf commented Jan 21, 2020

jmarolf commented Jan 21, 2020

JoeRobich Jan 21, 2020

Choose a reason for hiding this comment

ghost Jan 23, 2020

Choose a reason for hiding this comment

JoeRobich Feb 3, 2020

Choose a reason for hiding this comment

JoeRobich commented Jan 21, 2020

jmarolf commented Feb 1, 2020

JoeRobich left a comment

Choose a reason for hiding this comment

JoeRobich Feb 3, 2020

Choose a reason for hiding this comment

JoeRobich left a comment

Choose a reason for hiding this comment

jmarolf commented Feb 5, 2020

ghost commented Feb 5, 2020

format-379: Add `--report` command line argument to export json format report to given directory #495

format-379: Add `--report` command line argument to export json format report to given directory #495

ghost commented Jan 21, 2020 •

edited by ghost

Loading

dnfclas commented Jan 21, 2020 •

edited

Loading