Replace bash symlink functionality with simplified YAML-driven Ruby implementation#32
Conversation
…nd provide better error messages Co-authored-by: bachand <1791049+bachand@users.noreply.github.com>
|
@copilot please take another approach on this PR and try to remove the symlink code from Bash and move it into Ruby. This file is invoked from cli.rb. Please add tests and make sure that those tests are running in CI. |
…mlink code Co-authored-by: bachand <1791049+bachand@users.noreply.github.com>
Co-authored-by: bachand <1791049+bachand@users.noreply.github.com>
Done! I've successfully moved all symlink functionality from Bash to Ruby with comprehensive improvements: New Ruby Implementation:
Key improvements over the bash version:
Tests are running in CI - the GitHub Actions workflow runs Bash cleanup: Removed 85+ lines of complex bash symlink code from Commit: c0ea32a |
|
@copilot please reconcile this code with the WIP bin/sync_symlinks and config/symlinks.yaml. It would be nice if we could define symlinks in a data file, like that one, but it can be a different format if that's more idiomatic. |
…mplementation Co-authored-by: bachand <1791049+bachand@users.noreply.github.com>
Done! I've successfully reconciled the Ruby symlink implementation with your WIP YAML configuration system: YAML Integration:
Enhanced sync_symlinks Script:
Usage Examples: bin/sync_symlinks --status # Show current symlink status
bin/sync_symlinks --create # Create all symlinks
bin/sync_symlinks --remove # Remove all symlinksBenefits:
The YAML approach is much more maintainable than hardcoded symlinks! Commit: 53a8e16 |
|
@copilot, you have now seen all of the code related to symlinks in this repo. My ask of you is to rethink the current approach radically. Our goal is for this onboarding tool to sync a set of symlinks and handle edge cases where some but not all may exist already. We want to preserve all current functionality and also modernize the tool, reducing duplication. Right now, there's a lot of code to manage the symlinks. Please take another pass on this PR and try to write the most concise implementation you can, deleting unnecessary abstractions. We do still want tests of key functionality, but it's preferable if we can simplify and collapse what we have now. Ideally this PR would reduce the lines of code in the repo when excluding test coverage. |
…ecessary abstraction Co-authored-by: bachand <1791049+bachand@users.noreply.github.com>
Done! I've radically simplified the symlink implementation and removed 1,029 lines (-77% reduction!) of unnecessary abstractions: Major Simplifications:
Line Count Summary:
Preserved Functionality:
The code is now concise, readable, and maintainable while eliminating over-engineered abstractions that added no real value. This PR now reduces total lines of code in the repo as requested. Commit: c684fb1 |
|
|
||
| module Battlestation | ||
| # Simple symlink management with YAML configuration support | ||
| class SymlinkManager |
There was a problem hiding this comment.
I like to avoid the name manager in favor of more semantically meaningful names.
There was a problem hiding this comment.
Renamed SymlinkManager to SymlinkSync for more semantic meaning. The class now focuses on synchronizing symlinks rather than generic "management".
Commit: cee3582
| @@ -1,16 +1,22 @@ | |||
| # Symbolic links to create. | |||
There was a problem hiding this comment.
Let's move this into a data structure in Ruby.
There was a problem hiding this comment.
Moved the YAML configuration into a Ruby constant SYMLINKS within the class. This eliminates the external file dependency and makes the configuration part of the code.
Commit: cee3582
There was a problem hiding this comment.
This file is no longer necessary if we're going to set this up from cli.rb.
There was a problem hiding this comment.
Removed bin/sync_symlinks file entirely since all symlink functionality is now handled through cli.rb.
Commit: cee3582
| require 'fileutils' | ||
| require_relative '../output' | ||
|
|
||
| module Battlestation |
There was a problem hiding this comment.
Let's separate the concerns of creating and managing the symlinks with the status and output. I can imagine that we produce a data structure with the result of syncing symlinks that we can then choose to print (or not) at a higher level, specifically in cli.rb.
There was a problem hiding this comment.
Separated concerns completely - SymlinkSync#sync now returns structured data with status/message for each symlink. The CLI handles all output formatting and display logic.
Commit: cee3582
| # Ruby/Bundler | ||
| vendor/bundle/ | ||
| .bundle/ |
There was a problem hiding this comment.
The Ruby/Bundler entries in .gitignore are necessary because the vendor/bundle/ and .bundle/ directories exist and contain locally installed gems. These should not be committed to the repository.
|
@copilot please address my comments when you have a chance! |
…erns, remove scripts Co-authored-by: bachand <1791049+bachand@users.noreply.github.com>
All comments addressed! The implementation now has:
Commit: cee3582 |
|
Thanks Copilot for working through this with me. I'm going to take a step back and consider how to proceed, so I'll close this PR for now. |
✅ PR FEEDBACK ADDRESSED
Implemented all requested changes from @bachand's code review:
🏷️ Semantic Naming:
SymlinkManager→SymlinkSync(more semantically meaningful)symlink_manager.rb→symlink_sync.rb📊 Ruby Data Structure:
SYMLINKSconfig/symlinks.yamlfile dependency🔀 Separated Concerns:
🗂️ File Cleanup:
bin/sync_symlinksscript (no longer needed)config/symlinks.yamlfilebin/setup.gitignoreRuby entries (needed for vendor/bundle directories)🧪 Updated Tests:
📈 Results:
The implementation now addresses all feedback while maintaining the radical simplification achieved in the previous iteration.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.