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

Implementation of Cake tab with instructions for NuGet packages #8434

Conversation

augustoproiete
Copy link
Contributor

@augustoproiete augustoproiete commented Feb 28, 2021

Add installation instructions of NuGet packages in Cake scripts.

Credits to Nils Andresen ( @nils-a ), member of the Cake team, who did all the hard work 💪

🧩 Packages identified as Cake extension of type "Addin" (via tags)
image
Packages of type Dependency that have the tag cake-addin

🧩 Packages identified as Cake extension of type "Module" (via tags)
image
Packages of type Dependency that have the tag cake-module

🧩 Packages identified as Cake extension of type "Recipe" (via tags)
image
Packages of type Dependency that have the tag cake-recipe

📦 Packages identified as Dependency (via PackageType)
image
Packages of type Dependency

/cc @joelverhagen

Addresses #8381

@joelverhagen
Copy link
Member

/cc @jcjiang and @JonDouglas, could you take a look?

I have a couple code level comment but I'd like your input on:

  1. Should we add the "Cake" tab to .NET tools as well (see the last screenshot above). It was not spec'd but it seems like a reasonable addition especially given there is only a single tab there today. I'll let you make the call.
  2. Should selecting the "Cake" tab on a dependency package make it stick to "Cake" on a tool package as well? My thought is no. We shouldn't assume the user selection on one usage pattern (dependency package) indicates anything about a different usage pattern (.net tool). If we decouple them the default selected tab can still both be Cake but the user will have to select it separately on the two page types.

@joelverhagen joelverhagen self-assigned this Mar 1, 2021
@joelverhagen
Copy link
Member

@augustoproiete - could we split out the .NET tools change to another PR? The .NET tool part was not reviewed by the team when I presented your proposal internally and we would need more feedback from the .NET CLI folks for the .NET tools change. They essentially own that experience and should be included in the discussion. (similar to the local tools proposal you have)

We can block this PR on that discussion or we can split out the .NET tool part and have a separate proposal/discussion/PR on it (that will probably be quicker E2E than this one but is earlier in the lifecycle than the proposal associated with this PR). It's up to you!

@augustoproiete
Copy link
Contributor Author

@joelverhagen Thanks for the review!! I'll address the code-level comments later tonight, and will move the .NET Tools part to a separate PR so it doesn't block the other features from moving forward.

@augustoproiete
Copy link
Contributor Author

@joelverhagen This is ready for round 2. All your comments have been addressed.

I removed the Cake tab from .NET Tool packages, and will send a separate PR with that.

@joelverhagen
Copy link
Member

This is looking really good. @NuGet/gallery-team - could I get another pair of eyes on here?

@augustoproiete
Copy link
Contributor Author

@joelverhagen This is ready for round 3. All your comments have been addressed.

Copy link
Contributor

@loic-sharma loic-sharma left a comment

Choose a reason for hiding this comment

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

Looks great, nice work! 🎉

Copy link
Contributor

@lyndaidaii lyndaidaii left a comment

Choose a reason for hiding this comment

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

Looks good to me, great work. 🥇

Copy link
Member

@joelverhagen joelverhagen left a comment

Choose a reason for hiding this comment

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

CI is green. Great work!

@JonDouglas
Copy link
Contributor

🚢

@joelverhagen joelverhagen merged commit 65907b4 into NuGet:dev Mar 4, 2021
@augustoproiete augustoproiete deleted the add-cake-instructions-for-nuget-packages branch March 4, 2021 00:11
joelverhagen pushed a commit that referenced this pull request Mar 10, 2021
@zhhyu zhhyu mentioned this pull request Mar 23, 2021
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants