Spec proposal for opt-in feature where PrivateAssets flag option indepdent from IncludeAssets/ExcludeAssets options #12313
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
erdembayar
force-pushed
the
dev-eryondon-privateasset-vs-includeexclude-asset
branch
6 times, most recently
from
8113dea
to
1217510
Compare
erdembayar
force-pushed
the
dev-eryondon-privateasset-vs-includeexclude-asset
branch
from
1217510
to
0864d28
Compare
8 tasks
|
Fyi @AArnott |
zivkan
reviewed
Dec 8, 2022
zivkan
reviewed
Dec 8, 2022
nkolev92
reviewed
Dec 9, 2022
erdembayar
force-pushed
the
dev-eryondon-privateasset-vs-includeexclude-asset
branch
3 times, most recently
from
3cf6007
to
41db842
Compare
erdembayar
force-pushed
the
dev-eryondon-privateasset-vs-includeexclude-asset
branch
from
41db842
to
d231ef3
Compare
zivkan
reviewed
Dec 9, 2022
zivkan
reviewed
Dec 9, 2022
zivkan
reviewed
Dec 9, 2022
erdembayar
force-pushed
the
dev-eryondon-privateasset-vs-includeexclude-asset
branch
from
8aa4878
to
72bcb2c
Compare
AArnott
approved these changes
Apr 7, 2023
Co-authored-by: Andrew Arnott <andrewarnott@gmail.com>
Co-authored-by: Andrew Arnott <andrewarnott@gmail.com>
Co-authored-by: Andrew Arnott <andrewarnott@gmail.com>
erdembayar
commented
Apr 7, 2023
nkolev92
reviewed
Apr 8, 2023
There was a problem hiding this comment.
The proposal seems reasonable to me.
I do think that the proposal itself is still really difficult to read and pretty lengthy, so I left a bunch of feedback on that.
- I tried to call out as many of the difficulties I had going through the spec. I think it could be a lot shorter and communicate the same ideas.
- A lot of my comments apply to spec writing overall, so hopefully you find that helpful. Providing the right amount of detail requires going through the same design many times and often, re-reading the same text over and over, it just blurs, so it can be super easy to end up with lots of redundancies. The fact that it's been a while since I last read the spec is probably what allowed me to provide a lot of this feedback.
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\refissue\refissue.csproj" PrivateAssets="none" ExcludedAssetsFlow="true"/> |
There was a problem hiding this comment.
I think you've already communicated that they're independent. Adding length examples is probably best done in something shorter than a paragraph or a complete section.
I had to look hard at the examples to understand them, but in reality, they're really really close together.
If you say PrivateAssets and IncludeAssets are independent once, you don't need an example with different values of IncludeAssets, you just one where PrivateAssets and IncludeAssets conflict.
There was a problem hiding this comment.
Added the missing scenarios you mentioned in your previous review comments, so I added 2 more in this part of the spec. #12313 (comment)
Co-authored-by: Nikolche Kolev <nikolce1kolev@gmail.com>
erdembayar
force-pushed
the
dev-eryondon-privateasset-vs-includeexclude-asset
branch
from
30d1a77
to
0f4b052
Compare
Co-authored-by: Nikolche Kolev <nikolce1kolev@gmail.com>
erdembayar
force-pushed
the
dev-eryondon-privateasset-vs-includeexclude-asset
branch
from
ddd279f
to
6ac4464
Compare
erdembayar
force-pushed
the
dev-eryondon-privateasset-vs-includeexclude-asset
branch
from
6ac4464
to
1af7f27
Compare
ghost
closed this
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to: #6938
Description
Currently
Include/Excludeflag completely eclipsesPrivateAssetsflag when generating nupkg package in many cases. So, it doesn't let package dependencies flow to parent projects of consuming project. Here's a spec proposal for how to address with new opt-in feature property.Rendered