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

Pack localized assemblies. #1807

Merged
merged 1 commit into from Jul 14, 2016

Conversation

Projects
None yet
5 participants
@JohanLarsson
Contributor

JohanLarsson commented Jul 13, 2016

An attempt at #1806

Show outdated Hide outdated src/Paket.Core/ProjectFile.fs
let includeattribute = "Include"
this.ProjectNode
|> getDescendants "EmbeddedResource"
|> List.filter (fun n -> n |> hasAttribute includeattribute)

This comment has been minimized.

@vasily-kirichenko

vasily-kirichenko Jul 13, 2016

Contributor

why not point free?

@vasily-kirichenko

vasily-kirichenko Jul 13, 2016

Contributor

why not point free?

This comment has been minimized.

@JohanLarsson

JohanLarsson Jul 13, 2016

Contributor

What does it mean?

@JohanLarsson

JohanLarsson Jul 13, 2016

Contributor

What does it mean?

This comment has been minimized.

@theimowski

theimowski Jul 13, 2016

Member

means (fun n -> n |> hasAttribute includeattribute) is the same as (hasAttribute includeattribute)

@theimowski

theimowski Jul 13, 2016

Member

means (fun n -> n |> hasAttribute includeattribute) is the same as (hasAttribute includeattribute)

This comment has been minimized.

@JohanLarsson

JohanLarsson Jul 13, 2016

Contributor

Thanks.

@JohanLarsson

JohanLarsson Jul 13, 2016

Contributor

Thanks.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jul 13, 2016

Member

Awesome! Only one important thing left: docs. ;-)

Could you please add a small bit about this auto-packing of satelitte assemblies?

Member

forki commented Jul 13, 2016

Awesome! Only one important thing left: docs. ;-)

Could you please add a small bit about this auto-packing of satelitte assemblies?

Show outdated Hide outdated src/Paket.Core/PackageMetaData.fs
let template =
satelliteDlls
|> Seq.fold (fun template dll -> addFile (fst dll).FullName (snd dll) template) template

This comment has been minimized.

@smoothdeveloper

smoothdeveloper Jul 13, 2016

Contributor

you could decompose dll here:

 |> Seq.fold (fun template (dllFile, targetDir) -> addFile dllFile.FullName targetDir template) template
@smoothdeveloper

smoothdeveloper Jul 13, 2016

Contributor

you could decompose dll here:

 |> Seq.fold (fun template (dllFile, targetDir) -> addFile dllFile.FullName targetDir template) template

This comment has been minimized.

@JohanLarsson

JohanLarsson Jul 13, 2016

Contributor

Nice, I was stuck on that one for a while, used a fuzzying process.

@JohanLarsson

JohanLarsson Jul 13, 2016

Contributor

Nice, I was stuck on that one for a while, used a fuzzying process.

Show outdated Hide outdated src/Paket.Core/PackageMetaData.fs
let satelliteDlls =
seq {
for project in projects do

This comment has been minimized.

@smoothdeveloper

smoothdeveloper Jul 13, 2016

Contributor

(minor/style) not sure of general convention, but CE block should be indented (or put seq { at end of previous line)

@smoothdeveloper

smoothdeveloper Jul 13, 2016

Contributor

(minor/style) not sure of general convention, but CE block should be indented (or put seq { at end of previous line)

Show outdated Hide outdated src/Paket.Core/PackageMetaData.fs
let fileName = Path.Combine(outputDir, language, satelliteAssemblyName)
if File.Exists fileName then
let satelliteTargetDir = Path.Combine(targetDir, language)
yield (FileInfo(fileName), satelliteTargetDir)

This comment has been minimized.

@smoothdeveloper

smoothdeveloper Jul 13, 2016

Contributor

parens are uneeded:

yield (FileInfo filename), satelliteTargetDir

maybe even check if remaining are needed

@smoothdeveloper

smoothdeveloper Jul 13, 2016

Contributor

parens are uneeded:

yield (FileInfo filename), satelliteTargetDir

maybe even check if remaining are needed

This comment has been minimized.

@JohanLarsson

JohanLarsson Jul 13, 2016

Contributor

I could probably remove the FileInfo as it is just an allocation as we have already checked Exists

@JohanLarsson

JohanLarsson Jul 13, 2016

Contributor

I could probably remove the FileInfo as it is just an allocation as we have already checked Exists

Show outdated Hide outdated src/Paket.Core/ProjectFile.fs
this.ProjectNode
|> getDescendants "EmbeddedResource"
|> List.filter (hasAttribute includeattribute)
|> List.map (fun n -> n.Attributes.[includeattribute])

This comment has been minimized.

@smoothdeveloper

smoothdeveloper Jul 13, 2016

Contributor

maybe define tryGetAttribute

let tryGetAttributeValue name node = 
    if hasAttribute name node then
        Some node.Attributes.[name].Value
    else
        None

then you can replace List.filter/List.map/List.map with

|> List.choose (tryGetAttributeValue"Include")
@smoothdeveloper

smoothdeveloper Jul 13, 2016

Contributor

maybe define tryGetAttribute

let tryGetAttributeValue name node = 
    if hasAttribute name node then
        Some node.Attributes.[name].Value
    else
        None

then you can replace List.filter/List.map/List.map with

|> List.choose (tryGetAttributeValue"Include")
Show outdated Hide outdated src/Paket.Core/ProjectFile.fs
|> List.filter (hasAttribute includeattribute)
|> List.map (fun n -> n.Attributes.[includeattribute])
|> List.map (fun a -> a.Value)
|> List.map (fun a -> Regex.Match(a, pattern, RegexOptions.ExplicitCapture))

This comment has been minimized.

@smoothdeveloper

smoothdeveloper Jul 13, 2016

Contributor

it feels like a helper local function to handle the regex logic (return Some "the identified language" or None with single function call) would avoid mixing list operation with multistep processing of regex (which is more imperative thing).

@smoothdeveloper

smoothdeveloper Jul 13, 2016

Contributor

it feels like a helper local function to handle the regex logic (return Some "the identified language" or None with single function call) would avoid mixing list operation with multistep processing of regex (which is more imperative thing).

This comment has been minimized.

@JohanLarsson

JohanLarsson Jul 13, 2016

Contributor

Gah, I forgot about this refactoring.

@JohanLarsson

JohanLarsson Jul 13, 2016

Contributor

Gah, I forgot about this refactoring.

@JohanLarsson

This comment has been minimized.

Show comment
Hide comment
@JohanLarsson

JohanLarsson Jul 13, 2016

Contributor

@forki updated with a mention in docs.

Contributor

JohanLarsson commented Jul 13, 2016

@forki updated with a mention in docs.

@forki forki merged commit 0c32b9b into fsprojects:master Jul 14, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jul 14, 2016

Member

mhmm looks like it's broken on mono. can you please take a look?

Member

forki commented Jul 14, 2016

mhmm looks like it's broken on mono. can you please take a look?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jul 14, 2016

Member

we also got a regression, so I reverted that change for now. see #1810

Member

forki commented Jul 14, 2016

we also got a regression, so I reverted that change for now. see #1810

@JohanLarsson

This comment has been minimized.

Show comment
Hide comment
@JohanLarsson

JohanLarsson Jul 14, 2016

Contributor

I'll be mostly afk for a couple of days. Checking as soon as I can. Maybe outputting a warning is better than throwing if satellite dll is not found.

Contributor

JohanLarsson commented Jul 14, 2016

I'll be mostly afk for a couple of days. Checking as soon as I can. Maybe outputting a warning is better than throwing if satellite dll is not found.

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