-
Notifications
You must be signed in to change notification settings - Fork 11
Fix three onboarding issues #29
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
Conversation
When picking a non-symposium project, the user will now get a nice error message. Also fixed build script for people who are not Niko.
Bare repo clones don't set up all the references that full clones do. Since we want the root repo to be a bare clone, we have to do some of that setup manually.
You know...it might be nice if this worked.... |
nikomatsakis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code change themselves look good. I am unclear on how .kiro steering files are supposed to work -- in general I've been trying to use md as the place to store collected documents that will guide LLMs. I'd rather not "hardcode" to Kiro, kind of goes against Symposium's unopinionated design philosophy.
| @@ -0,0 +1,242 @@ | |||
| # Design Document | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So--- it's a bit unclear to me how these docs are intended to fit into the workflow. Is it intentional that you commit these? And how does it fit with a project where some people are using Kiro and some are not? This document in particular seems like it is specific to this PR and could quickly become outdated.
The more "Symposium-way" would be to edit and extend the mdbook's design section.
.kiro/steering/tech.md
Outdated
| @@ -0,0 +1,88 @@ | |||
| # Technology Stack | |||
|
|
|||
| ## Build System & Workspace | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems useful enough but it feels like it should probably just be in an AGENTS.md if we don't have one already,I forget.
|
|
||
|
|
||
| /// Execute a process and return results | ||
| private func executeProcess( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good 👍
symposium/macos-app/Sources/Symposium/Models/ProjectManager.swift
Outdated
Show resolved
Hide resolved
symposium/macos-app/Sources/Symposium/Models/ProjectManager.swift
Outdated
Show resolved
Hide resolved
| let process = Process() | ||
| process.executableURL = URL(fileURLWithPath: "/usr/bin/git") | ||
| process.arguments = ["rev-list", "--count", "\(branchName)", "--not", baseBranch] | ||
| let remoteBranch = "origin/\(baseBranch)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not thrilled about hardcoding origin here (and elsewhere) -- let's at least pull it into a constant somewhere.
|
|
||
| Logger.shared.log("ProjectManager[\(instanceId)]: No default branch configured, auto-detecting from git") | ||
|
|
||
| // Auto-detect origin's default branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: we really need to have tests for this kind of logic
Ack, shouldn't have committed these, mistake on my part, meant to put them in a gitignore |
Co-authored-by: Claude <[email protected]>
Co-authored-by: Claude <[email protected]>
- Add remoteName property to Project struct with 'origin' as default - Update ProjectManager to use project.remoteName instead of hardcoded 'origin' - Support for projects using different remote names (upstream, source, etc.) - Maintain backward compatibility with existing projects Co-authored-by: Claude <[email protected]>
nikomatsakis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM! At the moment, from what I can see, there is no GUI to edit the remote name, right? But you could edit the JSON by hand I guess.
Fixed three issues I ran into while onboarding
Let me know if you want any stylistic changes.