Source Control Navigator: Added support for viewing file changes. #596
Conversation
CodeEditModules/Modules/Git/src/Client/Models/ChangedFiles.swift
Outdated
Show resolved
Hide resolved
CodeEdit/NavigatorSidebar/SourceControlNavigator/SourceControlToolbar.swift
Outdated
Show resolved
Hide resolved
CodeEdit/NavigatorSidebar/SourceControlNavigator/Model/ChangesModel.swift
Outdated
Show resolved
Hide resolved
CodeEdit/NavigatorSidebar/SourceControlNavigator/Model/ChangesModel.swift
Outdated
Show resolved
Hide resolved
CodeEdit/NavigatorSidebar/SourceControlNavigator/Model/ChangesModel.swift
Outdated
Show resolved
Hide resolved
CodeEdit/NavigatorSidebar/SourceControlNavigator/Model/ChangesModel.swift
Outdated
Show resolved
Hide resolved
CodeEdit/NavigatorSidebar/SourceControlNavigator/Model/ChangesModel.swift
Outdated
Show resolved
Hide resolved
CodeEdit/NavigatorSidebar/SourceControlNavigator/SourceControlNavigatorView.swift
Show resolved
Hide resolved
CodeEdit/NavigatorSidebar/SourceControlNavigator/SourceControlNavigatorView.swift
Outdated
Show resolved
Hide resolved
CodeEdit/NavigatorSidebar/SourceControlNavigator/SourceControlSearchToolbar.swift
Outdated
Show resolved
Hide resolved
CodeEdit/NavigatorSidebar/SourceControlNavigator/SourceControlSearchToolbar.swift
Outdated
Show resolved
Hide resolved
| @@ -59,6 +59,25 @@ public extension GitClient { | |||
| } | |||
| } | |||
|
|
|||
| /// Displays paths that have differences between the index file and the current HEAD commit, | |||
There was a problem hiding this comment.
I would put this comment above the closure in the interface
There was a problem hiding this comment.
marked as unresolved since it wasn't changed
There was a problem hiding this comment.
Not enough information to do what you are asking for
There was a problem hiding this comment.
marked as unresolved since it wasn't changed
There was a problem hiding this comment.
What I'm suggesting to do here is moving this comment above the closure declaration which you can find in the Interface
There was a problem hiding this comment.
What interface... Direct me to the line
There was a problem hiding this comment.
| .map { line -> ChangedFiles in | ||
| let paramData = line.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| let parameters = paramData.components(separatedBy: " ") | ||
| return ChangedFiles(changeType: parameters[safe: 0] ?? "", |
There was a problem hiding this comment.
thoughts on not using optionals and checking that the array contains the value we expect? If not we can return nil and change this from being a map to a compactMap
There was a problem hiding this comment.
marked as unresolved since this was unchanged
There was a problem hiding this comment.
Not enough information to do what you are asking for
CodeEditModules/Modules/Git/src/Client/Models/ChangedFiles.swift
Outdated
Show resolved
Hide resolved
CodeEdit/NavigatorSidebar/SourceControlNavigator/SourceControlSearchToolbar.swift
Outdated
Show resolved
Hide resolved
CodeEdit/NavigatorSidebar/SourceControlNavigator/SourceControlSearchToolbar.swift
Outdated
Show resolved
Hide resolved
CodeEdit/NavigatorSidebar/SourceControlNavigator/Views/Repositories/RepositoriesView.swift
Show resolved
Hide resolved
CodeEditModules/Modules/Git/src/Client/Models/ChangedFiles.swift
Outdated
Show resolved
Hide resolved
| /// Initialize with a GitClient | ||
| /// - Parameter workspaceURL: the current workspace URL | ||
| /// | ||
| public init(workspaceURL: URL) { |
There was a problem hiding this comment.
is the workspaceURL here used only for initializing the GitClient ? If so we should inject the git client instead of the workspaceURL
There was a problem hiding this comment.
It's used for ChangedFileItemView to open files in the finder also
There was a problem hiding this comment.
marking as unresolved since it wasn't resolved. The ChangedFileItemView seems to already have a workspaceURL
| /// Initialize with GitClient | ||
| /// - Parameter gitClient: a GitClient | ||
| init(workspaceURL: URL) { | ||
| self.model = .init(workspaceURL: workspaceURL) |
There was a problem hiding this comment.
instead of passing the workspaceURL we can pass the SourceControlModel directly
There was a problem hiding this comment.
It's used for ChangedFileItemView to open files in the finder also
There was a problem hiding this comment.
could you indicate me in which file this is used please? It's a weird pattern
There was a problem hiding this comment.
seems that ChangedFileItemView has the workspaceURL too so it should be fine if we remove it from here
There was a problem hiding this comment.
If you read my comment right you would see I said it is used to open the finder at the file directory.
There was a problem hiding this comment.
@nanashili I definitely read your comment right, but a link to the actual reference might be helpful for the context. Thank you!
CodeEdit/NavigatorSidebar/SourceControlNavigator/Views/Changes/ChangesView.swift
Show resolved
Hide resolved
CodeEditModules/Modules/Git/src/Client/Models/ChangedFiles.swift
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| /// Returns the file name (e.g.: `Package.swift`) | ||
| public var fileName: String { |
There was a problem hiding this comment.
do we need to have a comptued property ? We can retrieve this info directly from the URL
There was a problem hiding this comment.
Yes because we use it do display on the list.
There was a problem hiding this comment.
please do not mark comments as resolved if not resolved
There was a problem hiding this comment.
when we display this on the list we could actually access the URL directly
There was a problem hiding this comment.
Do you want clean code or do you want a mess because that's what you are suggesting right now.
CodeEditModules/Modules/Git/src/Client/Models/ChangedFiles.swift
Outdated
Show resolved
Hide resolved
CodeEditModules/Modules/Git/src/Client/Models/ChangedFiles.swift
Outdated
Show resolved
Hide resolved
CodeEditModules/Modules/Git/src/Client/Models/ChangedFiles.swift
Outdated
Show resolved
Hide resolved
CodeEditModules/Modules/WorkspaceClient/src/Model/FileItem.swift
Outdated
Show resolved
Hide resolved
| .compactMap { line -> ChangedFiles in | ||
| let paramData = line.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| let parameters = paramData.components(separatedBy: " ") | ||
| guard let url = URL(string: parameters[safe: 1] ?? "") else { |
There was a problem hiding this comment.
since we are guarding for the url we shouldn't handle the optional with an empty string. we can return an error or nil if we can't find what we are looking for on the array
| } | ||
| return try output | ||
| .split(whereSeparator: \.isNewline) | ||
| .compactMap { line -> ChangedFiles in |
There was a problem hiding this comment.
it seems we are not returning nil anywhere else which means we don't have to use a compactMap
There was a problem hiding this comment.
Imma need you to decide now if you want this to be a map or compactMap...
There was a problem hiding this comment.
I gave you options. I suggested you to return a nil and use a compactMap but you decided to handle the optional and throw an error, in that case a compactMap is not needed. Here it is a good resource for this:
https://www.hackingwithswift.com/articles/205/whats-the-difference-between-map-flatmap-and-compactmap
|
|
||
| // swiftlint:disable identifier_name | ||
| public enum GitType: String, Codable { | ||
| case M = "M" |
There was a problem hiding this comment.
a couple of things here.
- Cases shouldn't be capitalized
- Each case should be more descriptive
- if the case is equal to the
rawValueyou don't have to specify the actual rawValue
|
|
||
| // swiftlint:disable identifier_name | ||
| public enum FileType: String { | ||
| case json = "json" |
There was a problem hiding this comment.
if the rawValue is equal to the actual case name we can omit the rawValue itself
|
Closing this PR since there is to much small issues that @MarcoCarnevali has with this PR |
Description
Added support for showing changed/created files in the currently opened workspace.
Related Issue
✨ Source Control Navigator #352
Checklist
Screenshots