Code health improvements #172
Conversation
So our filenames match our class names. :)
In FilesystemTransaction.
We could still do with more refactoring here to combine Install() and GetModuleContentsList(), which still share a lot of duplicate code.
- Added CachedFile() to return a file if cached. - IsCached() now defined in terms of CachedFile(). - Documentation, yay! For KSP-CKAN#90.
This stops a whole lot of whiny errors when building, but doesn't change functionality. Attentioning @ROMB and/or @AlexanderDzhoganov on this. I know it's safe to do this, but an OK that this is fine from a GUI development standpoint would be good.
- Both of these now require a file/directory - Added more tests. For KSP-CKAN#90
- KSP provides a .Cache attribute, for the cache for that instance. - ModuleInstaller.Cache proxies KSP.Cache - Lots of tests For KSP-CKAN#90
- KSPTests.cs -> KSP.cs - ModuleInstallerTest.cs -> ModuleInstaller.cs This also improves our NUnit out. For KSP-CKAN#90.
* 166_tweaks: GetModuleContentsList() now calls GenerateDefaultInstall() Throw TransactionalKrakens on double commit/rollback. FilesystemTransaction: Dispose of our scope before nulling it. Rename TransactionalFilesystem to FilesystemTransaction Well.. with a few minor tweaks This should work now :) Added '.NET TransactionalFileManager' to packages Conflicts: CKAN/CKAN/CKAN.csproj
Now we can find them from modules and un-opened files, as well as just stanzas and open zipfiles.
Also moved KSPManager tests to their own file.
- 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).
|
Also, |
This reverts commit ed84e82.
|
This is amazing, but please wait until we figure out the file locking issue before merging this. |
|
@AlexanderDzhoganov : D'oh, missed you on IRC again. My theory regarding the file locking issue is that while I believed that files would automatically close when the variables associated with them went out of scope, they don't actually close unless they're disposed off first. This can be done manually, but the preferred way is with a using statement, which guarantees the resource will be disposed of when it falls out of scope. Alternatively, it looks like Consequently, my expectation is that the reason why we're seeing file locking issues on Windows is because we're doing lots of opening of files, but very little closing of them. These changes:
If anything they should ease the file locking issues. I actually suspect that for zipfiles we need to do an explicit |
|
@AlexanderDzhoganov : Also, if we are going to be making a bunch of changes to try and solve the file locking issue, it would be really nice if we had a lot more test cases and much less code duplication when doing so, and these changes provide both of those. (In fact, they're almost exclusively those.) |
Code health improvements
|
Thank you! <3 |
Code health improvements
This PR increases the health of our code by adding lots of
tests and comments. In cases, code was refactored to use
existing functions rather than duplicating code.
See each commit message for details.
Includes PR #171