Skip to content
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.

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

Move common command line code into CommandLineUtils #201

Merged
merged 1 commit into from
Mar 22, 2017
Merged

Conversation

natemcmaster
Copy link

Remove duplication from our code.

We have duplicate implementations of this code scattered around a few projects. By putting htis here, we can remove the duplicate code from aspnet/DotNetTools, aspnet/EntityFramework.Tools, aspnet/BuildTools, and maybe more.

Resolves #173

cc @Eilon @muratg should we API review these before we add this?

@Eilon
Copy link
Member

Eilon commented Mar 16, 2017

Can we chat for a few minutes about the various command line changes being proposed (not just this change)?

@natemcmaster
Copy link
Author

⌚ until triage discussion next week.

@natemcmaster natemcmaster force-pushed the namc/cli-api branch 2 times, most recently from 770777c to d588798 Compare March 21, 2017 23:49
@natemcmaster
Copy link
Author

🆙 📅 rebased on #205. Now that CommandLineUtils is a .Sources package only, we shouldn't have to worry as much about API review on this. This is just putting code in a common location that we already have duplicated in a few places.

@natemcmaster
Copy link
Author

See also aspnet/DotNetTools#280

Copy link
Member

@Eilon Eilon left a comment

Choose a reason for hiding this comment

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

The change overall looks good. I'm just wondering why some of the files moved into a CommandLineApplication subfolder despite that not being in the namespace?

@natemcmaster
Copy link
Author

I'm just wondering why some of the files moved into a CommandLineApplication

I don't remember what I was thinking. Updated with that change reverted.

@Eilon
Copy link
Member

Eilon commented Mar 22, 2017

Re-approved 😄

@natemcmaster natemcmaster force-pushed the namc/cli-api branch 2 times, most recently from e182bd5 to a11ef27 Compare March 22, 2017 17:06
These APIs are commonly used in other ASP.NET Core tools. This change
puts the API into this shared library for use across all repos.
@natemcmaster natemcmaster merged commit 077dceb into dev Mar 22, 2017
@natemcmaster natemcmaster deleted the namc/cli-api branch March 22, 2017 17:26
@ghost ghost locked as resolved and limited conversation to collaborators May 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants