Refactor ApolloExtension of FileManager
#1999
Conversation
This is intended to reduce confusion with similarly named methods from the base class.
| @@ -4,118 +4,81 @@ import CommonCrypto | |||
| import ApolloUtils | |||
| #endif | |||
|
|
|||
| /// A protocol to decouple `ApolloExtension` from `FileManager`. Use it to build objects that can support | |||
| /// `ApolloExtension` behavior. | |||
| public protocol FileManagerProvider { | |||
There was a problem hiding this comment.
There is brittleness to this protocol in that if the FileManager API were to change it would no longer conform but in that case we'd need to update our extension anyways.
calvincestari
added
size/medium
codegen
| var isFolder = ObjCBool(false) | ||
| let exists = base.fileExists(atPath: path, isDirectory: &isFolder) | ||
| return exists && !isFolder.boolValue | ||
| public enum PathError: Swift.Error, LocalizedError, Equatable { |
There was a problem hiding this comment.
I don't know how I feel about this:
PathErroris an appropriate name for the kinds of errors being returned- There will now be a
PathErrorinApolloCodegenandFileManager/MockFileManager. They share similar errors but are crafted to be specific to their scope. - Adding the error to the scope of the extension should be OK since it's within the
apollonamespace but if the compiler takes issue with that then it'll create issues for us ifFileManagernatively getsPathError.
Overall I believe we need to move towards an ApolloCodegenLib scoped error enum that can encapsulate errors across all of code generation and can be extended as needed with things like recoverySuggestion, etc. For this PR however I'm inclined to leave this as-is and track the refactoring as another issue for 1.0.
There was a problem hiding this comment.
I created #2008 to track this.
There was a problem hiding this comment.
I generally have a lot of opinions on the structure of error APIs lol. It's something I nerd out on a little. So lets talk about this before starting work on #2008
| var isFolder = ObjCBool(false) | ||
| let exists = base.fileExists(atPath: path, isDirectory: &isFolder) | ||
| return exists && !isFolder.boolValue | ||
| public enum PathError: Swift.Error, LocalizedError, Equatable { |
There was a problem hiding this comment.
I generally have a lot of opinions on the structure of error APIs lol. It's something I nerd out on a little. So lets talk about this before starting work on #2008
It's expected behaviour not to overwrite directories and the opposite for files.
This is a fairly significant refactor of the
ApolloExtensiononFileManager. It was done with the purpose of supporting only what is needed/used in the extension ofFileManager, enable mock testing, and should make it easier to remove the CLI code.FileManager.deletereduced to one method and it always calls down to the file system.FileManagermethods.String) operations. Paths indicate a file system which better expresses the intent of the extension.FileManagerfromApolloExtensionto enable mocked testing.