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

Convert from nuget command #105

Merged
merged 3 commits into from
Sep 16, 2014

Conversation

theimowski
Copy link
Member

the convert-from-nuget command finds all packages.config files and based on them generates dependencies.file + converts packages.config to paket.references.
If the packages.config is solution-level, then it will be removed, and its deps will be written to dependencies.file
I also added some primitive logic to alter solution file, in order to remove solution-level packages.config.
dependencies.file for the moment will not be added to the solution.

I copied the pattern to find project files (_._proj).

@forki
Copy link
Member

forki commented Sep 15, 2014

Will test tomorrow. Pretty excited about this.

@forki forki merged commit 93df696 into fsprojects:master Sep 16, 2014
forki added a commit that referenced this pull request Sep 16, 2014
forki added a commit that referenced this pull request Sep 16, 2014
@theimowski theimowski deleted the convert_from_nuget_command branch September 16, 2014 07:41
@forki
Copy link
Member

forki commented Sep 16, 2014

Now this will happen:

tumblr_inline_mvcj2iqdog1raprkq

@theimowski
Copy link
Member Author

LOL

@forki
Copy link
Member

forki commented Sep 16, 2014

Please review my changes.

And I think we should add a parameter which disables the install process.

@theimowski
Copy link
Member Author

I'm ok with the changes - I wasn't really sure about the ~>, and filtering projects.
+1 for the optional paramater to disable install

@bartelink
Copy link
Member

@theimowski Wow, cool work!

@forki
Copy link
Member

forki commented Sep 17, 2014

Looks really cool: lefthandedgoat/canopy#166

@theimowski
Copy link
Member Author

Are we gonna save the World soon? :)

@forki
Copy link
Member

forki commented Sep 17, 2014

I encourage everyone to silently test as much projects as possible.
On Sep 17, 2014 2:15 PM, "Tomasz Heimowski" [email protected]
wrote:

Are we gonna save the World soon? :)


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

@bartelink
Copy link
Member

Everything works great for a tree with Webapi and tests.

In order of how much I believe they are actionable and/or would impress me, some small things:

  • Having Paket.references at the top of a project's Items in F# looks good to me as e.g. if I want to add xUnit. I'm adding a string immediately after I see it's not in the References (maybe this was already so?)
  • the insertions into Paket.dependencies were all quoted style (i.e. the generator should stop putting any " chars into the file
  • I had a Paket.dependencies and I didnt want it blindly overwritten (it was a superset). Maybe a --force would be nice or append?
  • Maybe it could tell me a full list of stuff it's going to do and let me OK it (or do nothing and required --force, i.e. operate in a -WhatIf mode normally)
  • my .nuget sln folder no longer had anything so it could have been deleted (and arguably the .nuget sln folder could be renamed .Paket
  • my .nuget dir no longer had anything so it could have been deleted (rather than just the package.config file)
  • I had hyprlinkr in a Paket.references but it wasn't picked up by convert (there was no conclusive version as Deps had just been overwritten)
  • the fact that the sorting in the .lock file is case sensitive makes ordering wierd
  • I was pretty sure the command was upgrade and I mistyped the name (vs convert)
  • I still think the lock` file would be better grouped by resolution batches

If anyone wants me to log or expand on any of the ideas, should. Otherwise I'll continue to sit here quietly being ignored :)

@forki
Copy link
Member

forki commented Sep 17, 2014

Thanks for testing.

I changed it to generate the new syntax.

Regarding the wording: I'm happy with convert since it doesn't blame nuget as outdated.

I don't think we need a whatif mode - git diff and git reset give exactly that.

Anyone interested in taking the delete stuff?

@bartelink
Copy link
Member

@forki I accept the -WhatIf being overkill and the workaround - was just enumerating possible options ;)

... but what about it bailing if there's a Paket.dependencies file and insisting on a --force ? (Might entail deferring closing of file until completion of whole process though as a Ctrl-C seems to leave an empty file now)

(I managed to overwrite stuff with an up arrow being the reason I re-ask)

I'll admit it's sooo neat that it's kind of irrelevant as retyping takes 2 seconds but it's not an ideal first experience if you're not fully awake and/or someone insists you just do it to be trying out a tool with a great, well-justified Just Works reputation and then end up with data loss.

@forki
Copy link
Member

forki commented Sep 17, 2014

Let's just assume the thingy is a "convert from nuget". Not a "convert from
something between nuget and paket". Will make everything much easier ;-)
On Sep 17, 2014 7:10 PM, "bartelink" [email protected] wrote:

@forki https://github.com/forki I accept the -WhatIf being overkill and
the workaround - was just enumerating possible options ;)

... but what about it bailing if there's a Paket.dependencies file and
insisting on a --force ? (Might entail deferring closing of file until
completion of whole process though as a Ctrl-C seems to leave an empty file
now)

(I managed to overwrite stuff with an up arrow being the reason I re-ask)

I'll admit it's sooo neat that it's kind of irrelevant as retyping
takes 2 seconds but it's not an ideal first experience if you're not fully
awake and/or someone insists you just do it to be trying out a tool with a
great, well-justified Just Works reputation and then end up with data loss.


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

@theimowski
Copy link
Member Author

I'll take care of deletion of empty .nuget dir. @bartelink - you mentioned that the solution folder also didn't get removed? If so, could you please share a repro? Will also add optional --no-install parameter as discused earlier.

@bartelink
Copy link
Member

@forki All for minimal but surely warning that you're about to overwrite a significant file like Paket.dependencies isnt too much?
@theimowski No, wasn't saying the sln folder didn't get removed (but rereading I can see it's pretty unclear!). The Paket.dependencies and Paket.lock live there (not sure if you put them there or whether I have but I like them as sln folder items). Was saying that all items in the .nuget folder are defunct - i.e. I dont have an exe or a targets. Hence the fact that my 2x Paket. files are in a .nuget folder looks a little wierd. Sorry - not explaining myself well!

@forki
Copy link
Member

forki commented Sep 17, 2014

Yes a message would really be important. Or we could simply let it fail if
the file already exist.
On Sep 17, 2014 9:37 PM, "bartelink" [email protected] wrote:

@forki https://github.com/forki All for minimal but surely warning that
you're about to overwrite a significant file like Paket.dependencies isnt
too much?
@theimowski https://github.com/theimowski No, wasn't saying the sln
folder didn't get removed (but rereading I can see it's pretty unclear!).
The Paket.dependencies and Paket.lock live there (not sure if you put
them there or whether I have but I like them as sln folder items). Was
saying that all items in the .nuget folder are defunct - i.e. I dont have
an exe or a targets. Hence the fact that my 2x Paket. files are in a
.nuget folder looks a little wierd. Sorry - not explaining myself well!


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

@bartelink
Copy link
Member

@forki Cool. Yes, I meant a message if file(s) already exist that then stops. Possibly with a --force option in case one is bumping into and interactively fixing inconsistencies in the course of doing the convert (though admittedly that's speculative (and defeatist!))

@forki
Copy link
Member

forki commented Sep 17, 2014

@theimowski can you please add this too?

@theimowski
Copy link
Member Author

Sure, so if paket.dependencies exists then warn, and stop program? And a --force flag to ignore if paket.dependencies exists?

@forki
Copy link
Member

forki commented Sep 17, 2014

Right. And the same for the references files.
On Sep 17, 2014 10:03 PM, "Tomasz Heimowski" [email protected]
wrote:

Sure, so if paket.dependencies exists then warn, and stop program? And a
--force flag to ignore if paket.dependencies exists?


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

forki added a commit that referenced this pull request Sep 18, 2014
@forki
Copy link
Member

forki commented Sep 18, 2014

@theimowski can you please take a look at https://github.com/Particular/NServiceBus/blob/release-5.0.0/src/nuget.config

I think if the file exists we need to extract the source feeds. Can you try to do this?

@theimowski
Copy link
Member Author

Will do

@forki
Copy link
Member

forki commented Sep 18, 2014

And are we doing this: e74a907 ?

@theimowski
Copy link
Member Author

Nope, the command deals only with packages.config on solution level

@forki
Copy link
Member

forki commented Sep 18, 2014

would be "nice-to-have"

2014-09-18 12:05 GMT+02:00 Tomasz Heimowski [email protected]:

Nope, the command deals only with packages.config on solution level


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

@theimowski
Copy link
Member Author

Regarding nuget.config: can we ignore activePackageSource and disabledPackageSources and just extract all sources from packageSources node for now?

@forki
Copy link
Member

forki commented Sep 18, 2014

Yep I think that's ok

forki added a commit that referenced this pull request Oct 29, 2014
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.

3 participants