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

Allow filtered updates of packages matching a regex #1178

Merged
merged 7 commits into from
Oct 30, 2015
Merged

Allow filtered updates of packages matching a regex #1178

merged 7 commits into from
Oct 30, 2015

Conversation

mavnn
Copy link
Contributor

@mavnn mavnn commented Oct 30, 2015

Especially useful for situations like "update all packages that start with my organisations prefix"

@forki
Copy link
Member

forki commented Oct 30, 2015

do you have a small sample for this? Then I could add a regression test

@mavnn
Copy link
Contributor Author

mavnn commented Oct 30, 2015

Of course.

Given dependency file:

source https://nuget.org/api/v2

nuget Castle.Windsor                           
nuget NUnit                             
nuget Microsoft.AspNet.WebApi.SelfHost

Then:

paket update nuget Microsoft.* --filter should update AspNet

paket update nuget [MN].* --filter will update NUnit and AspNet but leave Castle.Windsor untouched.

Without the --filter flag, package names are treated as exact matches. This is potentially important as . is a special character in regexs.

forki
forki reviewed Oct 30, 2015
View reviewed changes
src/Paket.Core/PackageResolver.fs
@@ -189,6 189,7 @@ type Resolved = {
type UpdateMode =
| UpdatePackage of GroupName * PackageName
Copy link
Member

Choose a reason for hiding this comment

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

can we remove that case and express it as UpdateFiltered of GroupName * PackageFilter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As in, escape .s and treat it as a filter? I think it would work. Hope your regression tests are good up to date though! :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Give me a few minutes and I'll see if all the tests pass.

@forki
Copy link
Member

forki commented Oct 30, 2015

image

@mavnn
Copy link
Contributor Author

mavnn commented Oct 30, 2015

My code obviously rules :)

@mavnn
Copy link
Contributor Author

mavnn commented Oct 30, 2015

Mind pushing that test to my master branch? I'll add it to the pull request.

@mavnn
Copy link
Contributor Author

mavnn commented Oct 30, 2015

Ah, ok - I'll grab it.

@forki
Copy link
Member

forki commented Oct 30, 2015

40ecd39

@forki
Copy link
Member

forki commented Oct 30, 2015

you need to run build.cmd runintegrationtests since we don't execute that target during standard build

@mavnn
Copy link
Contributor Author

mavnn commented Oct 30, 2015

Should having working unit and integration tests now

mavnn
mavnn reviewed Oct 30, 2015
View reviewed changes
tests/Paket.Tests/UpdateProcessSpecs.fs
|> ignore)
|> shouldFail

[<Test>]
let ``SelectiveUpdate does not update any package when package does not exist``() =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this test as it's expectation didn't seem meaningful anymore. With pattern based updates, a pattern that didn't match any packages should be a no-op, not an exception.

The update package command still checks that the specific named package exists before calling UpdateFiltered

@forki
Copy link
Member

forki commented Oct 30, 2015

Could also please write one or two samples in the docs of the update command?

@mavnn mavnn merged commit 27f7f2f into fsprojects:master Oct 30, 2015
@mavnn
Copy link
Contributor Author

mavnn commented Oct 30, 2015

@forki crap. Push to wrong remote by mistake. Sorry.

@mavnn
Copy link
Contributor Author

mavnn commented Oct 30, 2015

/me goes off to change default push repository

@forki
Copy link
Member

forki commented Oct 30, 2015

no worries ;-)

forki added a commit that referenced this pull request Oct 30, 2015
@forki
Create more regression tests for #1178
6a5641a
@pms1969 pms1969 mentioned this pull request Jan 12, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mavnn @forki