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

Add prerelease option to dotnet.exe add package command #9699

Merged
merged 5 commits into from
Jul 6, 2020

Conversation

martinrrm
Copy link
Contributor

Design for new prerelease option in dotnet.exe add

Design for new prerelease option in dotnet.exe add
@martinrrm martinrrm requested a review from nkolev92 June 18, 2020 22:21
@ghost
Copy link

ghost commented Jun 18, 2020

CLA assistant check
All CLA requirements met.

designs/Prerelease-Option.md Outdated Show resolved Hide resolved
designs/Prerelease-Option.md Outdated Show resolved Hide resolved
designs/Prerelease-Option.md Outdated Show resolved Hide resolved
designs/Prerelease-Option.md Outdated Show resolved Hide resolved
designs/Prerelease-Option.md Outdated Show resolved Hide resolved
| `dotnet.exe add package --prerelease` | 3.3.1-preview.3 | latest version of package |
| `dotnet.exe add package --prerelease --version 3.0.0` | error | The user cannot use this commands at the same time |

```package 2.0.0
Copy link
Member

Choose a reason for hiding this comment

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

nit:
when you format put it on a new line.

Copy link
Member

Choose a reason for hiding this comment

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

A couple of questions/observations:

  1. Using the —prerelease option should get the latest version including prerelease. Correct?
  2. I find the name of the option a bit long. Can we also enable —pre may in addition to —prerelease? - a nit

Copy link
Member

Choose a reason for hiding this comment

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

  1. Yes. Similar to how the PM UI behaves. If you have the prerelease filter we show you the latest version if it's prerelease.

  2. We just opted for the full name cause I haven't seen option names not being the exact word. We can't really do --pre as short hand. A short option must be one letter only.
    So we either have --prerelease or --pre.

Copy link
Contributor

@chrisraygill chrisraygill Jun 25, 2020

Choose a reason for hiding this comment

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

Personally, I'd rather go with the options that is the most instinctual. So if I forgot what the options were but I wanted to enable prereleases, my instinct would be to add --prerelease not --pre even tho it's shorter. I think intuitiveness > saving a couple key strokes.

designs/Prerelease-Option.md Show resolved Hide resolved
designs/Prerelease-Option.md Show resolved Hide resolved
@zivkan
Copy link
Member

zivkan commented Jun 19, 2020

Please improve the PR title, as when looking at the pull request list, I have no idea what this is adding prerelease option to.

@martinrrm martinrrm changed the title Add prerelease option Add prerelease option to dotnet.exe add package command Jun 19, 2020
@nkolev92 nkolev92 self-requested a review June 19, 2020 23:49
designs/Prerelease-Option.md Show resolved Hide resolved
@nkolev92
Copy link
Member

nkolev92 commented Jun 20, 2020

@chgill-MSFT @JonDouglas Can we get some PM eyes on this?

@KathleenDollard @dsplaisted @wli3 @marcpopMSFT some CLI eyes on this?

Packages available

```
package 2.0.0
Copy link

Choose a reason for hiding this comment

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

Is 0.9 considered prerelease?
given versions:
0.8
0.9

with --prerelease -> 0.9
without -> fail ?

Copy link
Member

Choose a reason for hiding this comment

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

I don’t think so and @nkolev92 can confirm. The definition of prerelease is a version with “-“ in it.

Copy link
Member

Choose a reason for hiding this comment

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

Nope, as @anangaur mentioned, prerelease requires a - in the version.
0.9 is just a weird stable :D

For reference: https://nugettoolsdev.azurewebsites.net/5.5.0-preview.2.6382/parse-version?version=0.9

Choose a reason for hiding this comment

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

Is there any formatting that includes the - for a non-preview version or doesn't have the - for a preview version? Is that formatting enforced in other places as well and documented?

Copy link
Member

@nkolev92 nkolev92 Jun 22, 2020

Choose a reason for hiding this comment

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

Is there any formatting that includes the - for a non-preview version or doesn't have the - for a preview version?

Nope, that's per the semver specification.
Documented here: https://docs.microsoft.com/en-us/nuget/concepts/package-versioning

Copy link

Choose a reason for hiding this comment

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

Looks like I can restore a package without 1.0 with *. So that should be fine

<PackageReference Include="xunit.analyzers" Version="*" />

@wli3
Copy link

wli3 commented Jun 22, 2020

Is there anyway to get prerelease using csproj? Or dotnet tool still cannot get prerelease packages. (one of the top asks) Note, dotnet tool still uses a temp project which has something like

<PackageReference Include="my-tool" Version="*" />

@chrisraygill
Copy link
Contributor

Is there anyway to get prerelease using csproj? Or dotnet tool still cannot get prerelease packages. (one of the top asks) Note, dotnet tool still uses a temp project which has something like

<PackageReference Include="my-tool" Version="*" />

In the csproj you can specify latest version including prerelease with *-*. Something like 5.*-* also works to get the latest version including prelease in the version 5 range.

Does that answer your question @wli3?

@nkolev92
Copy link
Member

What @chgill-MSFT said.

See #912 was fixed in 5.6.

@wli3
Copy link

wli3 commented Jun 23, 2020

ah, that was in already. Great!

@nkolev92
Copy link
Member

@chgill-MSFT @JonDouglas

You guys good with this design?
Think the only topic of potential contention is the option name. @anangaur suggested it might be too long, but personally I think it's ok to be explicit.


## Goals

Add an option to the command `add package` to enable installing latest prerelease version.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just nitpick, but I think the goal should be framed as helping a customer accomplish a "job to be done." In this case the goal should be something like:

"Allow dotnet CLI users to easily install the latest version of NuGet package, including prereleases. This can be accomplished by adding an option to the existing add package that enables installing prerelease versions."

designs/Prerelease-Option.md Show resolved Hide resolved
Comment on lines 46 to 49
package 2.0.0
package 3.0.0
package 3.3.1-preview.3
package 3.4.0
Copy link
Contributor

Choose a reason for hiding this comment

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

These examples are kind of confusing because the full command is dotnet add package <packageId>. So using a an example package named "package" makes it hard to grok. Maybe using Newtonsoft.Json would work better?

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe something like Consoto.Library to reduce confusion of versions of Newtonsoft.Json that don't actually exist on nuget.org.

@chrisraygill
Copy link
Contributor

chrisraygill commented Jun 25, 2020

@chgill-MSFT @JonDouglas

You guys good with this design?
Think the only topic of potential contention is the option name. @anangaur suggested it might be too long, but personally I think it's ok to be explicit.

Personally, I'm fine with --prerelease especially since it's the wording we use elsewhere in NuGet so it'll be easy to remember. Added a relevant comment to the comment thread with Anand.

@nkolev92
Copy link
Member

@chgill-MSFT No precedent afaik.
nuget.exe install uses the complete spelling, https://docs.microsoft.com/en-us/nuget/reference/cli-reference/cli-ref-install.

|---------|--------------------|------------------|-------------|
| `dotnet.exe add Contoso.Library` | Contoso.Library 2.0.0 <br> Contoso.Library 3.0.0 <br> Contoso.Library 3.3.1-preview.3 <br> | 3.0.0 | latest stable version of package |
| `dotnet.exe add Contoso.Library --prerelease` | Contoso.Library 2.0.0 <br> Contoso.Library 3.0.0 <br> Contoso.Library 3.3.1-preview.3 <br> | 3.3.1-preview.3 | latest version of package |
| `dotnet.exe add Contoso.Library --prerelease --version 3.0.0` | Contoso.Library 2.0.0 <br> Contoso.Library 3.0.0 <br> Contoso.Library 3.3.1-preview.3 <br> | error | The user cannot use the `--prerelease` and `--version` commands at the same time |
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we error for dotnet.exe add Contoso.Library --prerelease --version 3.0.0 or just ignore the --prerelease and let it work since the intent to install version 3.0.0 seems clear? I think that exact scenario is less likely, but I could potentially see someone doing dotnet.exe add Contoso.Library --prerelease --version 3.0.0-preview thinking they need --prerelease to install it.

Erroring makes sense if that's the standard behavior of dotnet.exe in these kinds of scenarios.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's ok to error when the options provided are confusing and do not make sense.

The https://docs.microsoft.com/en-us/nuget/reference/cli-reference/cli-ref-trusted-signers command throws a bunch of, hey you can't combine these switches errors.

@nkolev92
Copy link
Member

nkolev92 commented Jul 1, 2020

Last calls for feedback :) @chgill-MSFT @JonDouglas @anangaur

@martinrrm martinrrm merged commit b6a1811 into dev Jul 6, 2020
@nkolev92 nkolev92 deleted the dev-martinrrm-prereleaseOption branch September 1, 2020 19:05
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.

7 participants