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

Don't allow empty string as description - take II #1831

Merged
merged 2 commits into from Jul 25, 2016

Conversation

Projects
None yet
4 participants
@inosik
Contributor

inosik commented Jul 25, 2016

Seems like I forgot this in #1728.

However, I also changed the error message to show the merged metadata, not only the infos from paket.template. I think that was the intent anyways.

inosik added some commits Jul 25, 2016

Fix error message for missing metadata
With this change, the error message will show the merged (template file
and assembly infos) metadata, not only the information from the template
file.

@forki forki merged commit 0080050 into fsprojects:master Jul 25, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@inosik inosik deleted the inosik:bugfix/empty-description branch Jul 25, 2016

@richard-green

This comment has been minimized.

Show comment
Hide comment
@richard-green

richard-green Jul 25, 2016

FYI, this has broken our builds. Probably intentionally, but still!

richard-green commented Jul 25, 2016

FYI, this has broken our builds. Probably intentionally, but still!

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jul 25, 2016

Member

I can revert if that makes sense

Member

forki commented Jul 25, 2016

I can revert if that makes sense

@richard-green

This comment has been minimized.

Show comment
Hide comment
@richard-green

richard-green Jul 25, 2016

Hi - for now I've pinned our build to use 3.9.1, was more just an informational / warning note to others :-)

richard-green commented Jul 25, 2016

Hi - for now I've pinned our build to use 3.9.1, was more just an informational / warning note to others :-)

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jul 25, 2016

Member

I reverted it. and releases 3.9.2 is in progress.

@inosik I think we should raise a warning instead.

Member

forki commented Jul 25, 2016

I reverted it. and releases 3.9.2 is in progress.

@inosik I think we should raise a warning instead.

@inosik

This comment has been minimized.

Show comment
Hide comment
@inosik

inosik Jul 25, 2016

Contributor

Sorry for the inconvenience, @richard-green, but I think this change is needed. Packages with any of id, version, authors or description, missing, are practically unusable outside of Paket. That's every tool which uses NuGet.Core, that being NuGet command line, NuGet server, Klondike, Squirrel, Octopus Deploy and Visual Studio. Probably some more.

@biehlermi and I had a hard time figuring out why NuGet based test adapters didn't work in a project of ours. The reason was a package created with Paket, which broke VSs discovery process.

See also themotleyfool/Klondike#158.

Contributor

inosik commented Jul 25, 2016

Sorry for the inconvenience, @richard-green, but I think this change is needed. Packages with any of id, version, authors or description, missing, are practically unusable outside of Paket. That's every tool which uses NuGet.Core, that being NuGet command line, NuGet server, Klondike, Squirrel, Octopus Deploy and Visual Studio. Probably some more.

@biehlermi and I had a hard time figuring out why NuGet based test adapters didn't work in a project of ours. The reason was a package created with Paket, which broke VSs discovery process.

See also themotleyfool/Klondike#158.

@inosik

This comment has been minimized.

Show comment
Hide comment
@inosik

inosik Jul 25, 2016

Contributor

@forki, I would rather let Paket fail, because a warning message is easily overseen in the output of a build script. It also clearly indicates the cause of the failure, instead of an error later in the process, like in themotleyfool/Klondike#158. But I also agree that this is kind of breaking.

Contributor

inosik commented Jul 25, 2016

@forki, I would rather let Paket fail, because a warning message is easily overseen in the output of a build script. It also clearly indicates the cause of the failure, instead of an error later in the process, like in themotleyfool/Klondike#158. But I also agree that this is kind of breaking.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jul 25, 2016

Member

We could still give it a default description generated from the id and
raise a warning. This would be a soft fix and just silently repair stuff

On Jul 25, 2016 8:04 PM, "Ilja Nosik" notifications@github.com wrote:

@forki https://github.com/forki, I would rather let Paket fail, because
a warning message is easily overseen in the output of a build script. It
also clearly indicates the cause of the failure, instead of an error later
in the process, like in themotleyfool/Klondike#158
https://github.com/themotleyfool/Klondike/issues/158. But I also agree
that this is kind of breaking.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1831 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNN8roYEQgECqq56D-VOVvmmz-de3ks5qZPqUgaJpZM4JUHLP
.

Member

forki commented Jul 25, 2016

We could still give it a default description generated from the id and
raise a warning. This would be a soft fix and just silently repair stuff

On Jul 25, 2016 8:04 PM, "Ilja Nosik" notifications@github.com wrote:

@forki https://github.com/forki, I would rather let Paket fail, because
a warning message is easily overseen in the output of a build script. It
also clearly indicates the cause of the failure, instead of an error later
in the process, like in themotleyfool/Klondike#158
https://github.com/themotleyfool/Klondike/issues/158. But I also agree
that this is kind of breaking.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1831 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNN8roYEQgECqq56D-VOVvmmz-de3ks5qZPqUgaJpZM4JUHLP
.

@inosik

This comment has been minimized.

Show comment
Hide comment
@inosik

inosik Jul 26, 2016

Contributor
Contributor

inosik commented Jul 26, 2016

@BADF00D

This comment has been minimized.

Show comment
Hide comment
@BADF00D

BADF00D Jul 26, 2016

As @inosik mentioned in https://github.com/themotleyfool/Klondike/issues/158, description is an obligatory field. I'm not a friend of soft fixes. They are almost always a pitfall for anyone who does not know about this.
By the way. We use a paket on a build server. As far as there is no error, nobody ever cares about log files from builds.

BADF00D commented Jul 26, 2016

As @inosik mentioned in https://github.com/themotleyfool/Klondike/issues/158, description is an obligatory field. I'm not a friend of soft fixes. They are almost always a pitfall for anyone who does not know about this.
By the way. We use a paket on a build server. As far as there is no error, nobody ever cares about log files from builds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment