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

Convert-from-nuget should simplify framework restrictions if possible #1159

Merged
merged 4 commits into from Oct 22, 2015

Conversation

Projects
None yet
2 participants
@forki
Member

forki commented Oct 22, 2015

implements #1107

/cc @theimowski could you please take a short look? Thx

Show outdated Hide outdated src/Paket.Core/NugetConvert.fs
@@ -296,7 +296,9 @@ let createDependenciesFileR (rootDirectory : DirectoryInfo) nugetEnv mode =
Paket.DependenciesFile(DependenciesFileParser.parseDependenciesFile dependenciesFileName newLines))
if File.Exists dependenciesFileName then read() else create()
match (if File.Exists dependenciesFileName then read() else create()) with

This comment has been minimized.

@theimowski

theimowski Oct 22, 2015

Member

use Trial.lift (map) ?

@theimowski

theimowski Oct 22, 2015

Member

use Trial.lift (map) ?

@@ -66,7 +66,7 @@ let simplify interactive environment = trial {
let! lockFile = environment |> PaketEnv.ensureLockFileExists
let flatLookup = lockFile.GetDependencyLookupTable()
let dependenciesFileRef = ref environment.DependenciesFile
let dependenciesFileRef = ref (environment.DependenciesFile.SimplifyFrameworkRestrictions())

This comment has been minimized.

@theimowski

theimowski Oct 22, 2015

Member

this will happen also on paket simplify command?

@theimowski

theimowski Oct 22, 2015

Member

this will happen also on paket simplify command?

This comment has been minimized.

@forki

forki Oct 22, 2015

Member

on simplify and convert

@forki

forki Oct 22, 2015

Member

on simplify and convert

@@ -398,6 +398,27 @@ type DependenciesFile(fileName,groups:Map<GroupName,DependenciesGroup>, textRepr
member __.Groups = groups
member this.SimplifyFrameworkRestrictions() =
let transform (dependenciesFile:DependenciesFile) (group:DependenciesGroup) =
if group.Options.Settings.FrameworkRestrictions <> [] then dependenciesFile else

This comment has been minimized.

@theimowski

theimowski Oct 22, 2015

Member

what happens when there are global restrictions and standalone restrictions per package? The latter override global?

@theimowski

theimowski Oct 22, 2015

Member

what happens when there are global restrictions and standalone restrictions per package? The latter override global?

This comment has been minimized.

@forki

forki Oct 22, 2015

Member

we don't do anything in this case.
during normal run both restrictions work together as two filters.

@forki

forki Oct 22, 2015

Member

we don't do anything in this case.
during normal run both restrictions work together as two filters.

Show outdated Hide outdated src/Paket.Core/DependenciesFile.fs
let sameRequirements =
rest |> Seq.forall (fun p' -> p'.Settings.FrameworkRestrictions = package.Settings.FrameworkRestrictions)
if not sameRequirements || package.Settings.FrameworkRestrictions = [] then dependenciesFile else

This comment has been minimized.

@theimowski

theimowski Oct 22, 2015

Member

must check if restrictions are empty? does it harm if you add empty restrictions globally?

@theimowski

theimowski Oct 22, 2015

Member

must check if restrictions are empty? does it harm if you add empty restrictions globally?

This comment has been minimized.

@forki

forki Oct 22, 2015

Member

yes we create a stupid line

@forki

forki Oct 22, 2015

Member

yes we create a stupid line

@theimowski

This comment has been minimized.

Show comment
Hide comment
@theimowski

theimowski Oct 22, 2015

Member

LGTM, wondering if SimplifyFrameworkRestrictions could be made a bit more idiomatic, but that's probably just some cosmetics

Member

theimowski commented Oct 22, 2015

LGTM, wondering if SimplifyFrameworkRestrictions could be made a bit more idiomatic, but that's probably just some cosmetics

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 22, 2015

Member

What would you call it?
On Oct 22, 2015 8:55 PM, "Tomasz Heimowski" notifications@github.com
wrote:

LGTM, wondering if SimplifyFrameworkRestrictions could be made a bit more
idiomatic, but that's probably just some cosmetics


Reply to this email directly or view it on GitHub
#1159 (comment).

Member

forki commented Oct 22, 2015

What would you call it?
On Oct 22, 2015 8:55 PM, "Tomasz Heimowski" notifications@github.com
wrote:

LGTM, wondering if SimplifyFrameworkRestrictions could be made a bit more
idiomatic, but that's probably just some cosmetics


Reply to this email directly or view it on GitHub
#1159 (comment).

@theimowski

This comment has been minimized.

Show comment
Hide comment
@theimowski

theimowski Oct 22, 2015

Member

don't mean name itself, but rather the logic - maybe replace ifs with pattern matching where possible - but as I said it's just cosmetics, not crucial

Member

theimowski commented Oct 22, 2015

don't mean name itself, but rather the logic - maybe replace ifs with pattern matching where possible - but as I said it's just cosmetics, not crucial

forki added some commits Oct 22, 2015

@forki forki merged commit d5c4273 into master Oct 22, 2015

2 of 3 checks passed

continuous-integration/appveyor AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@theimowski

This comment has been minimized.

Show comment
Hide comment
@theimowski

theimowski Oct 22, 2015

Member

👍

Member

theimowski commented Oct 22, 2015

👍

@forki forki deleted the restrictionsimplyfier branch Oct 23, 2015

@forki

This comment has been minimized.

Show comment
Hide comment
Member

forki commented Oct 23, 2015

@theimowski

This comment has been minimized.

Show comment
Hide comment
@theimowski

theimowski Oct 23, 2015

Member

neat!

Member

theimowski commented Oct 23, 2015

neat!

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