-
Notifications
You must be signed in to change notification settings - Fork 0
Remove remote uri from core stack domain #390
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
c6e5d80 to
4d8bc07
Compare
5d36139 to
1a9cf92
Compare
8d06d1f to
46d4572
Compare
| var stacks2 = repository.GetStacks(); | ||
| var stack2Model = new Model.Stack("Stack2", "main", []); | ||
| repository.AddStack(stack2Model); | ||
| repository.RemoveStack(stack2Model); |
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.
Need to check actual stacks or save here
| await Task.CompletedTask; | ||
|
|
||
| var configPath = stackConfig.GetConfigPath(); | ||
| var configPath = dataStore.GetConfigPath(); |
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.
Should change this to be an Open method or something similar.
| namespace Stack.Persistence; | ||
|
|
||
| public record StackData(List<Model.Stack> Stacks); | ||
| public record StackDataItem(string Name, string RemoteUri, string SourceBranch, List<StackBranchItem> Branches); |
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.
Might want better names than these
a8ac267 to
4d4ca5e
Compare
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.
Pull Request Overview
This PR refactors the stack persistence layer by removing remote URI from the core stack domain model and introducing separate types for configuration file persistence. The refactor separates concerns between the domain model (which no longer includes remote URI) and the data storage layer (which maintains remote URI for repository filtering).
Key changes:
- Removed
RemoteUriparameter from coreStackmodel - Introduced
StackDataItemandStackBranchItemas separate persistence types - Added mapping logic between domain and persistence models in
StackRepository - Renamed
IStackConfig/FileStackConfigtoIStackDataStore/FileStackDataStore
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Stack/Model/Stack.cs | Removed RemoteUri parameter from Stack record |
| src/Stack/Persistence/StackRepository.cs | Added mapping between domain and persistence models, renamed dependencies |
| src/Stack/Persistence/FileStackDataStore.cs | Renamed from FileStackConfig, updated to work with new StackDataItem types |
| src/Stack/Infrastructure/HostApplicationBuilderExtensions.cs | Updated DI registration to use new interface name |
| src/Stack/Commands/Stack/NewStackCommand.cs | Updated to create Stack without RemoteUri |
| src/Stack/Commands/Config/OpenConfigCommand.cs | Updated to use new IStackDataStore interface |
| Test files | Updated all test files to remove RemoteUri from Stack constructors and use new types |
| { | ||
| stackData.Value.Stacks.Remove(stack); | ||
| var remoteUri = GetRemoteUri(); | ||
| var stackToRemove = stackData.Value.Stacks.FirstOrDefault(s => s.Name == stack.Name && s.RemoteUri == remoteUri); |
Copilot
AI
Oct 17, 2025
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.
Consider using StringComparison.OrdinalIgnoreCase for the RemoteUri comparison to maintain consistency with the filtering logic in GetStacks() method.
| var stackToRemove = stackData.Value.Stacks.FirstOrDefault(s => s.Name == stack.Name && s.RemoteUri == remoteUri); | |
| var stackToRemove = stackData.Value.Stacks.FirstOrDefault(s => s.Name == stack.Name && s.RemoteUri.Equals(remoteUri, StringComparison.OrdinalIgnoreCase)); |
| var stackToRemove = stackData.Value.Stacks.FirstOrDefault(s => s.Name == stack.Name && s.RemoteUri == remoteUri); | ||
| if (stackToRemove == null) | ||
| { | ||
| throw new InvalidOperationException($"Stack '{stack.Name}' does not exist in the current repository."); |
Copilot
AI
Oct 17, 2025
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 error message mentions 'current repository' but the actual issue could be either that the stack doesn't exist or that it exists but for a different remote URI. Consider making the error message more specific about which condition failed.
| throw new InvalidOperationException($"Stack '{stack.Name}' does not exist in the current repository."); | |
| // Check if stack exists for a different remote URI | |
| var stackWithName = stackData.Value.Stacks.FirstOrDefault(s => s.Name == stack.Name); | |
| if (stackWithName != null) | |
| { | |
| throw new InvalidOperationException( | |
| $"Stack '{stack.Name}' exists, but for a different remote URI ('{stackWithName.RemoteUri}'). " + | |
| $"Current remote URI: '{remoteUri}'."); | |
| } | |
| else | |
| { | |
| throw new InvalidOperationException($"Stack '{stack.Name}' does not exist."); | |
| } |
Removes Remote Uri from the core stack domain and introduces separate types for the config file.
Part of a series: