Skip to content

Rewrite dotnet-getdocument and GetDocument.Insider to retrieve all documents in one go #10667

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 3, 2019
Merged

Rewrite dotnet-getdocument and GetDocument.Insider to retrieve all documents in one go #10667

merged 3 commits into from
Jun 3, 2019

Conversation

dougbu
Copy link
Contributor

@dougbu dougbu commented May 30, 2019

  • Add Web Application project support for build-time OpenAPI document generation #8242 1 of 2
  • save a cache file listing all retrieved documents
  • remove .NET Core App 2.0 support
  • remove ServiceProjectReferenceMetadata.targets, related Project class, and searches for a project
    • tools will run within a project and get needed information from project on command line
  • roll framework forward in both tools to expand their applicability

nits:

  • refactor methods in GetDocumentCommandWorker
  • reorder option addition for consistency and to place --help at the top of help output
  • consolidate information about method signatures at top of GetDocumentCommandWorker
  • consolidate try / catch blocks in GetDocumentCommandWorker

@dougbu dougbu requested review from pranavkm and rynowak May 30, 2019 22:16
@dougbu dougbu added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 31, 2019
@dougbu dougbu mentioned this pull request May 31, 2019
Merged
javiercn
javiercn approved these changes May 31, 2019
View reviewed changes
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

Pranav asked me to help him look at this.

Overall I think looks good. I left a few comments. Mostly I would strongly recommend you to reuse the logic for finding the app entry point, as we share that everywhere.

I'm not sure about the constraints this tool has, but there are other things that are a bit concerning to me, like reading the deps file or the runtime.config.json file.

I also don't see any tests for this PR, not sure if its because there are already tests and didn't require changes or because there aren't any. If its the latter, I think there should be at least one, as this tool does a lot of things that are prone to integration errors.

That said, I don't see anything that strikes me as wrong here.

src/Mvc/GetDocumentInsider/src/Commands/GetDocumentCommandWorker.cs Outdated

// Write out the cache file.
var stream = File.Create(context.FileListPath);
using var writer = new StreamWriter(stream) { AutoFlush = true };
Copy link
Member

Choose a reason for hiding this comment

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

+5 Pranav points

rynowak
rynowak reviewed May 31, 2019
View reviewed changes
src/Mvc/GetDocumentInsider/src/Commands/GetDocumentCommand.cs
_output = command.Option("--output <Path>", Resources.OutputDescription);
_service = command.Option("--service <QualifiedName>", Resources.FormatServiceDescription(FallbackService));
_fileListPath = command.Option("--file-list <Path>", Resources.FileListDescription);
_output = command.Option("--output <Directory>", Resources.OutputDescription);
Copy link
Member

Choose a reason for hiding this comment

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

example usage of this for the unfamiliar?

Copy link
Member

Choose a reason for hiding this comment

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

Also, is this something that needs to be reviewed in detail? or is this an implementation detail of the MSBuild functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an implementation detail of the MSBuild functionality seen in #10669 and that PR shows how the tool should be used

src/Mvc/dotnet-getdocument/src/Commands/InvokeCommand.cs
args.Add(project.TargetPath);
args.Add(assemblyPath);
args.Add("--project");
args.Add(projectName);
Copy link
Member

Choose a reason for hiding this comment

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

I'm probably not the best person to review the inside-man stuff. I've never really known how this works 😕

@rynowak
Copy link
Member

rynowak commented May 31, 2019

The general spirit of the changes here makes sense to me and sees like a good enhancement. Due to the diffs being what they are, I'm not really sure if I'm reviewing old code or new code, but I still found some things that seem worthy of a look.

@dougbu dougbu changed the title Rewrite dotnet-getdocument and GetDocumentCommandInsider to retrieve all documents in one go Rewrite dotnet-getdocument and GetDocument.Insider to retrieve all documents in one go Jun 1, 2019
@dougbu
Copy link
Contributor Author

dougbu commented Jun 1, 2019

@javiercn I realize I missed your high-level comments, sorry:

things that are a bit concerning to me, like reading the deps file or the runtime.config.json file.

None of the code reads either of these files though it does scan the project assets file for probing paths. The dotnet command line doesn't have a --please-also-use-these-assets option.

See similar code in the dotnet-ef tool: https://github.com/aspnet/EntityFrameworkCore/blob/078449fd1363bc3d79778a4f726262e2ced30ae5/src/dotnet-ef/RootCommand.cs#L127-L139 So, fairly battle-tested

I also don't see any tests for this PR

This feature has some engineering debt I'll be catching up on in the next milestone. #4914 covers the test part of that.

dougbu added 3 commits June 2, 2019 19:21
@dougbu
Rewrite dotnet-getdocument and GetDocumentCommandInsider to retrieve …
e18693a 
…all documents in one go

- #8242 1 of 2
- save a cache file listing all retrieved documents
- remove .NET Core App 2.0 support
- remove ServiceProjectReferenceMetadata.targets, related `Project` class, and searches for a project
  - tools will run within a project and get needed information from project on command line
- roll framework forward in both tools to expand their applicability

nits:
- refactor methods in `GetDocumentCommandWorker`
- reorder option addition for consistency and to place `--help` at the top of help output
- consolidate information about method signatures at top of `GetDocumentCommandWorker`
- consolidate `try` / `catch` blocks in `GetDocumentCommandWorker`
@dougbu
Address PR comments
4974770 
- use Microsoft.Extensions.HostFactoryResolver.Sources
  - remove `GetMethod(...)` parameters that aren't needed anymore
  - remove Microsoft.AspNetCore.Hosting.Abstractions dependency
- remove `Flush()` and `AutoFlush = true`
- use `Exception.ToString()` and `String.Replace(...)`
- move unnecessary lines out of a `try` block

nits:
- "retrieving" -> "generating"
- shorten the lifespan of a `Task`
- ensure GetDocument.Insider exit codes are unique
- make a few more `string`s `const`
- fold a few expressions over fewer lines
@dougbu
!fixup! Shorten lifetime of a StreamWriter
d1130bf 
- ensure it is flushed before underlying stream is read
@dougbu dougbu force-pushed the dougbu/add.server-side.8242.1 branch from 426880e to d1130bf Compare June 3, 2019 02:23
dougbu
dougbu commented Jun 3, 2019
View reviewed changes
Copy link
Contributor Author

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

🆙:date; to address more comments than I expected

@dougbu dougbu merged commit 4c8ca0b into master Jun 3, 2019
@ghost ghost deleted the dougbu/add.server-side.8242.1 branch June 3, 2019 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rynowak rynowak rynowak left review comments

@javiercn javiercn javiercn approved these changes

@pranavkm pranavkm Awaiting requested review from pranavkm

Assignees
No one assigned
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dougbu @rynowak @javiercn