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

Some improvements #3090

Merged
merged 13 commits into from
Feb 28, 2018
Merged

Some improvements #3090

merged 13 commits into from
Feb 28, 2018

Conversation

matthid
Copy link
Member

@matthid matthid commented Feb 27, 2018

No description provided.

if checkCredentials (url, Some auth) then
Some auth
else
failwithf "Credentials from authentication store for %s are invalid" source
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this still fails, but a bit later and with one request less

@forki forki mentioned this pull request Feb 28, 2018
let inline getPackageIdAttribute (pf:ProjectFile) (node:XmlNode) =
node |> getAttribute "Include"
|> Option.orElseWith (fun _ -> node |> getAttribute "Update")
|> Option.defaultWith (fun _ -> failwithf "project file '%s' contains a reference without 'Include' or 'Update' attribute" pf.FileName)
Copy link
Member

Choose a reason for hiding this comment

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

that's a weird technique. throwing in defaultWith!? Seriously?

Copy link
Member Author

Choose a reason for hiding this comment

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

What else do you suggest? The only thing it does: Give a better error message in case all other cases fail and defaultWith has the signature I need for this here ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, I "default" "with" an error message. It doesn't even sound too bad. Is the code better with a match?

for problem in parseProblems |> Seq.map (fun x -> x.AsMessage) do
Logging.traceWarnfn "Problem: %s" problem

printfn "Restriction: %s" restrictionRaw
Copy link
Member

Choose a reason for hiding this comment

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

please don't use printfn directly. tracefn all the things

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, but but just to make it clear: This is in fact program output no logging

@@ -776,6 +794,7 @@ let handleCommand silent command =
| GenerateLoadScripts r -> processCommand silent generateLoadScripts r
| GenerateNuspec r -> processCommand silent generateNuspec r
| Why r -> processCommand silent why r
| Restriction r -> processCommand silent restriction r
Copy link
Member Author

Choose a reason for hiding this comment

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

@forki I can remove that from the PR if you don't like it, the rest are bug-fixes

Copy link
Member

Choose a reason for hiding this comment

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

no it's fine, but needs docs

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the question, I saw it more like a debug helper for us, but we can make it a feature (honestly I doubt if the regular user can do something with the output)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add some lines :)

@@ -0,0 +1,36 @@
# paket why
Copy link
Member Author

Choose a reason for hiding this comment

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

oh :)

@forki
Copy link
Member

forki commented Feb 28, 2018

it's red.
anc can you please merge master?

@@ -89,7 +89,7 @@
</When>
</Choose>
<Choose>
<When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And ($(TargetFrameworkVersion) == 'v4.5.1' Or $(TargetFrameworkVersion) == 'v4.5.2' Or $(TargetFrameworkVersion) == 'v4.5.3')">
<When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And ($(TargetFrameworkVersion) == 'v4.5' Or $(TargetFrameworkVersion) == 'v4.5.1' Or $(TargetFrameworkVersion) == 'v4.5.2' Or $(TargetFrameworkVersion) == 'v4.5.3')">
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what this PR has to do with this change. but it looks correct?

Copy link
Member

Choose a reason for hiding this comment

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

yes guess so

@forki forki merged commit b63178e into master Feb 28, 2018
@forki forki deleted the some_improvements branch February 28, 2018 15:17
@matthid
Copy link
Member Author

matthid commented Feb 28, 2018

So I got it green finally?

@forki
Copy link
Member

forki commented Feb 28, 2018

no it did not. yet

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.

2 participants