-
Notifications
You must be signed in to change notification settings - Fork 29
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
Update packages #161
Update packages #161
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever you think @sungam3r
@@ -6,7 +6,7 @@ | |||
</PropertyGroup> | |||
|
|||
<ItemGroup> | |||
<PackageReference Include="GraphQL" Version="5.2.0" /> | |||
<PackageReference Include="GraphQL" Version="$(GraphQLTestVersion)" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect. This should be the minimum version supported by the project. This is the way it will be built when packaged.
The tests should run with the test project having a reference to GraphQL using GraphQLVersion to simulate having a newer version installed than the project was built for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see the GraphQL.Authorization repo for a properly configured sample
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect. This should be the minimum version supported by the project. This is the way it will be built when packaged.
For "normal" project yes. For 0.x.x projects as one here my intent the opposite - update to the latest available for 0.7 with all bugfixes and features available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand and I'm okay if we have the "minimum" version as 5.3.1 in this case as an 0.x "beta" version. I just want to make sure that the build process is set up correctly. All looks good now. Feel free to bump the minimum version up if you want, or leave it as-is.
Update to latest and greatest bits. Relates to #160 . @Shane32 I propose to release v0.7 after merge. We did not release up-do-date package after #126.