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.

Sign up for GitHub

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

Jump to bottom

Copy missing changes from legacy NuGet helpers #1596

Merged
merged 1 commit into from
Jun 16, 2017
Merged

Copy missing changes from legacy NuGet helpers #1596

merged 1 commit into from
Jun 16, 2017

Conversation

zvirja
Copy link
Contributor

@zvirja zvirja commented Jun 15, 2017

The following PRs were abandoned during the NuGet helpers migration to a new project: #1530, #1549 and #1551. This PR copies that changes.

P.S. @matthid Please review the other recent PRs and ensure that their changes were migrated as well 😉

@zvirja
Copy link
Contributor Author

zvirja commented Jun 15, 2017

It's strange that build failed... Could you advice what is the reason? I haven't modified the FakeLib project, while it seems to be a reason.

@matthid
Copy link
Member

matthid commented Jun 15, 2017

Don't worry about it. Travis was broken for some time now because of a mono bug.
I think this particular instance is because we use xbuild instead of msbuild. This might go away after I bootstrap the build to alpha-10

@zvirja
Copy link
Contributor Author

zvirja commented Jun 15, 2017

Thanks. In this case will be looking forward to the code review 😉

matthid
matthid approved these changes Jun 15, 2017
View reviewed changes
Copy link
Member

@matthid matthid left a comment

Choose a reason for hiding this comment

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

Since this module is considered legacy (ie. people should use paket :P) I'm fine with accepting this as-is.

src/app/Fake.DotNet.NuGet/NuGet.fs
with exn ->
if exn.InnerException <> null then exn.Message + "\r\n" + exn.InnerException.Message else exn.Message
|> replaceAccessKeys parameters
|> failwith
Copy link
Member

Choose a reason for hiding this comment

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

Since fsprojects/Paket#2423 I'm no longer a huge fan of failwith in a general with clause.
But it's not clear how we can remove the access keys in a secure way. They probably shouldn't be part of the error message in the first place if possible :/

Copy link
Contributor Author

@zvirja zvirja Jun 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yep, probably we should find more graceful solution as I also don't like those e. Message + e.InnerException.Message. However, currently I'd prefer to make this PR simple and just copy the missing changes.

@matthid matthid merged commit 15e1b74 into fsprojects:master Jun 16, 2017
@matthid
Copy link
Member

matthid commented Jun 16, 2017

Thanks!

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

@matthid matthid matthid approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zvirja @matthid