Improvements on #24#30
Conversation
``` $ swift build -c release --arch arm64 --arch x86 warning: 'stellar': Invalid Resource 'Tests': File not found. ```
albertodebortoli
force-pushed
the
feature/13-stellar-env-alberto
branch
from
8eb1d95 to
17a673e
Compare
albertodebortoli
force-pushed
the
feature/13-stellar-env-alberto
branch
from
17a673e to
696d0a9
Compare
.github/workflows/run-tests.yml
Outdated
| matrix: | ||
| macos: ['macos-12'] | ||
| xcode: ['14.1.0'] | ||
| macos: ['macos-13'] |
There was a problem hiding this comment.
I'm not too confident about this anymore as GitHub Actions has been hanging for more than 3 hours 🤔
|
@albertodebortoli, great idea to move the To summarize, you were unable to test the following:
Right? |
scripts/install_private.sh
Outdated
| ASSET_URL=$(ruby ./assets_url.rb "${JSON}") | ||
|
|
||
| ohai "Downloading stellarenv [$ASSET_URL]" | ||
| ASSETS_URL=$(ruby ./assets_url.rb "${JSON}") |
There was a problem hiding this comment.
Are you sure? It just returns the URL of a single asset, the URL of the StellarEnv asset.
There was a problem hiding this comment.
No, I'm not sure :D
I thought it was desired to have it in line with the file. Maybe asset_url.rb (singular) then?
|
|
||
| // Unzip the file | ||
| try Shell.shared.unzip(fileURL: remoteFileURL, destinationURL: installURL) | ||
| NSWorkspace.shared.activateFileViewerSelecting([installURL]) |
There was a problem hiding this comment.
Probably we should disable it or make it dependent on a --debug command or similar.
There was a problem hiding this comment.
So that the file is opened in Finder only if --debug is used? I was thinking the same.
| if let version { | ||
| url = url.appendingPathComponent(version) | ||
| /// - Returns: a URL to the folder containing the installed versions | ||
| func systemVersionsLocation() throws -> URL { |
There was a problem hiding this comment.
Maybe CLIToolsVersions() is a better name to be consistent with the other code?
There was a problem hiding this comment.
Since they all end with either 'location' or 'url' and return URL, maybe we could name these 3 methods:
func homeStellarLocation(subfolder: String? = nil) -> URL
func cliVersionsLocation() throws -> URL
func cliLocation(for version: String) throws -> URL
| import Foundation | ||
| import StellarCore | ||
|
|
||
| @main |
There was a problem hiding this comment.
I removed this code on the first refactor some days ago (on #24), but it broke the forward operation. Did you notice the comment on main.swift? Have you found a fix on it/does forward still work?
There was a problem hiding this comment.
I saw the comment in main.swift and I think I misunderstood it. That's a bummer, we need to find a way around this or stellarenv won't leverage Argument Parser. I'm confident we will given that Tuist does it.
I've reverted 33f6b08 for now.
3 participants
I would like to suggest some improvements iteratively as I go through the code of PR #24 as it's quite big and I'm wrapping my head around every file.
Not many changes but I strongly advise inspecting each commit individually as the titles will help.
This PR addresses the comments left on #24.
Thanks @malcommac <3