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

Added '.NET Transactional File Manager' support, removed our implementation #166

Conversation

AlexanderDzhoganov
Copy link
Member

Closes #161

@pjf pjf mentioned this pull request Oct 25, 2014
}
else
{
contents.AddRange(FindAllFiles(zipfile));
Copy link
Member

Choose a reason for hiding this comment

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

Uh oh, we already have a method for doing this. GenerateDefaultInstall(). It's not the same as all the files in the archive; instead, we look for the top-most directory that has the same name as the module identifier, and install that.

This is really my fault for not factoring out all the code into something which lets you supply a module and a zipfile, and get back the files which would be installed. (That was kinda what FindInstallableFiles() was meant to do, but it existed before we had default installs).

I'll be sending you a PR on your PR, but it should be a pretty straightforward one. :)

  • Add FindInstallableFiles(Module, Zipfile) or analogue thereof.

Copy link
Member

Choose a reason for hiding this comment

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

I'm a terrible person, I didn't write something that gives back all the installable files from a Module and its Zipfile. I did patch it so it doesn't need FindAllFiles, and does match what would be installed. I also added a TODO. :)

@pjf
Copy link
Member

pjf commented Oct 25, 2014

The sheer number of lines of code deleted in this PR fills me with joy. Thank you. <3

@pjf
Copy link
Member

pjf commented Oct 26, 2014

@AlexanderDzhoganov , I've opened #171 and I'm closing this PR. #171 includes all of your changes, but has my own tweaks on top (which are relatively minor).

@pjf pjf closed this Oct 26, 2014
@pjf pjf removed the pull request label Oct 26, 2014
pjf added a commit to pjf/CKAN that referenced this pull request Oct 26, 2014
- Now a static method, for easier testing and less coupling.
- Has many method signatures. Can find files from module or stanza,
  zipfile or filename.
- Improved GetModuleContentsList() to use new FindInstallableFiles()

TODO: Still refactor everything use to use FindInstallableFiles()
(eg: Install()).

For KSP-CKAN#90, relates to KSP-CKAN#166 (which is contained by KSP-CKAN#171).
RichardLake pushed a commit to RichardLake/CKAN that referenced this pull request May 30, 2015
- Now a static method, for easier testing and less coupling.
- Has many method signatures. Can find files from module or stanza,
  zipfile or filename.
- Improved GetModuleContentsList() to use new FindInstallableFiles()

TODO: Still refactor everything use to use FindInstallableFiles()
(eg: Install()).

For KSP-CKAN#90, relates to KSP-CKAN#166 (which is contained by KSP-CKAN#171).
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.

Transactional file managers?
2 participants