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

Pull request/refactor update process #843

Merged

Conversation

cr7pt0gr4ph7
Copy link
Contributor

paket restore --only-referenced

As the title says: This pull requests makes it possible to say:

paket restore --only-referenced

which is the same as:

paket restore --references-files <all paket.references files that paket install would find>

paket install--only-referenced

Consequently, it also adds:

paket install --only-referenced

which works analogous to paket restore --only-referenced.

Refactorings

To make such additions easier in the future, InstallProcess and UpdateProcess were refactored to use the Parameter Object pattern for the different flags (Force, Hard, WithBindingRedirects).
The naming for the new record types InstallerOptions and SmartInstallOptions is not optimal, so I'm open for feedback.

Testing

I've not yet added tests for the new functionality. I'd be thankful if you could point me to the correct files where tests for InstallProcess and UpdateProcess sould be added.

Compatibility

Dependencies/PublicAPI.fs has been changed in a backwards compatible way by adding two new public members. The changes to InstallProcess and UpateProcess are not backwards compatible, but this is nearly unavoidable due to the quirks of F#.

@forki
Copy link
Member

forki commented May 28, 2015

Cool. You also need to add the parameter to the update command
On May 28, 2015 10:58 AM, "Lukas Waslowski" [email protected]
wrote:

paket restore --only-referenced

As the title says: This pull requests makes it possible to say:

paket restore --only-referenced

which is the same as:

paket restore --references-files

paket install--only-referenced

Consequently, it also adds:

paket install --only-referenced

which works analogous to paket restore --only-referenced.
Refactorings

To make such additions easier in the future, InstallProcess and
UpdateProcess were refactored to use the Parameter Object pattern for the
different flags (Force, Hard, WithBindingRedirects).
The naming for the new record types InstallerOptions and
SmartInstallOptions is not optimal, so I'm open for feedback.
Testing

I've not yet added tests for the new functionality. I'd be thankful if you
could point me to the correct files where tests for InstallProcess and
UpdateProcess sould be added.
Compatibility

Dependencies/PublicAPI.fs has been changed in a backwards compatible way
by adding two new public members. The changes to InstallProcess and
UpateProcess are not backwards compatible, but this is nearly unavoidable

due to the quirks of F#.

You can view, comment on, or merge this pull request online at:

#843
Commit Summary

  • Refactor UpdateProcess by introducing CommonOptions.
  • Other formatting fixes for UpdateProcess.
  • Refactor InstallProcess by introducing CommonOptions into its API.
  • Other formatting fixes for InstallProcess.
  • Remove UpdateProcess.SmartInstall and rename SmartInstallNew to
    SmartInstall.
  • Remove UpdateProcess.UpdatePackage and rename UpdatePackageNew to
    UpdatePackage.
  • Remove UpdateProcess.Update and rename UpdateNew to Update.
  • Remove InstallProcess.Install and rename InstallNew to Install.
  • Remove UpdateProcess.InstallIntoProjects and rename
    InstallIntoProjectsNew to InstallIntoProjects.
  • Refactor AddProcess by introducing CommonOptions.
  • Other formatting fixes for AddProcess.
  • Rename CommonOptions to InstallerOptions (the better name
    InstallOptions is already taken).
  • Add --only-referenced switches for "paket install" and "paket
    restore".
  • Other formatting fixes for PublicAPI.

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#843.

@cr7pt0gr4ph7
Copy link
Contributor Author

I'll look into the paket update command, and see if I can add the flag.
I also think that some code duplication between InstallProcess and UpdateProcess can be removed, but I would do another pull request for that.

@forki
Copy link
Member

forki commented May 28, 2015

Probably also needed in add command (just to keep it aligned)

@cr7pt0gr4ph7
Copy link
Contributor Author

The paket add command should also have the --redirects flags, like the other commands that install packages.

@forki
Copy link
Member

forki commented May 28, 2015

Correct. Would love to receive a pull request
On May 28, 2015 11:08 AM, "Lukas Waslowski" [email protected]
wrote:

The paket add command should also have the --redirects flags, like the
other commands that install packages.


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

@cr7pt0gr4ph7
Copy link
Contributor Author

paket remove needs --only-referenced and --redirects, too.

@cr7pt0gr4ph7
Copy link
Contributor Author

The problem with Dependencies is that the parameter lists grow longer and longer, as more flags are added. I'll just add the flags to the parameter lists for now; any solution to the problem of the longer parameter lists will break binary compatibility anyway (though source compatibility might be retained).

One possible solution would be do to it like FAKE:

// define test dlls
let testDlls = !! (testDir + "/Test.*.dll")

Target "xUnitTest" (fun _ ->
    testDlls
        |> xUnit (fun p -> 
            {p with 
                ShadowCopy = false;
                HtmlOutput = true;
                XmlOutput = true;
                OutputDir = testDir })
)

What do you think of the syntax of the xUnit function?

One important thing to know would be if you want to achieve binary backwards compatibility (which is hard in F#, escpecially for record types, as far as I know), or only source level backwards compatibility like FAKE?

UPDATE: I've created Issue #844 (Clean up public API of Dependencies) to keep track of this problem.

@forki
Copy link
Member

forki commented May 28, 2015

we don't need binary backwards compat. There is only Paket.VisualStudio as consumer at the moment.

@cr7pt0gr4ph7
Copy link
Contributor Author

Okay, then I'll just add more parameters to Dependencies, and then look into simplifying the API using the pattern from the code sample above.

@cr7pt0gr4ph7
Copy link
Contributor Author

After looking a bit into adding --only-referenced to paket add, paket update and paket remove, I'd say that it makes more sense to do the refactoring of Dependencies first, to limit code duplication.

As it currently stands, paket add --only-referenced (and paket remove --only-referenced) can be simulated using paket add --no-install followed by paket restore --only-referenced.

This is also a hint that much of the code from AddProcess, RemoveProcess and so on can unified by separating the required operations into the three phases:

  1. Modify paket.dependencies / paket.references / paket.lock / ...`,
  2. Download packages as required,
  3. Apply changes to *.csproj files,
    Of which (2) (and maybe (3)) can be implemented using shared code..

Because the necessary modifications are a bit of another story (and because I don't want this pull request to get huge), I'd vote to merge this pull request first.
The other reason is that I need paket install --only-referenced in my productive work, so I'd limit further additions to this pull request to the ones that are required to get it merged.

forki added a commit that referenced this pull request May 28, 2015
…e-process

Pull request/refactor update process
@forki forki merged commit 472ef61 into fsprojects:master May 28, 2015
@forki
Copy link
Member

forki commented May 28, 2015

Very cool. Need to try some things.

fast

@cr7pt0gr4ph7
Copy link
Contributor Author

Thank you! That was very fast.

@forki
Copy link
Member

forki commented May 28, 2015

I think this broke the default behaviour.

@forki
Copy link
Member

forki commented May 28, 2015

i think I found the issue

@forki
Copy link
Member

forki commented May 28, 2015

ok it's released in 1.7.1

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