Skip to content

Commit 3eece9c

Browse files
committed
👷 Improve CI config
1 parent ec63989 commit 3eece9c

File tree

4 files changed

+129
-15
lines changed

4 files changed

+129
-15
lines changed

.github/COPILOT_INSTRUCTIONS.md

Lines changed: 123 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,34 @@ When `replace_string_in_file` fails with "Could not find matching text":
101101

102102
## API Conventions
103103

104+
### Forward Compatibility with **options
105+
106+
**CRITICAL DESIGN PRINCIPLE**: All constructors and public API methods that accept keyword arguments MUST include `**options` (or similar catch-all) as the final parameter to capture unknown options.
107+
108+
**CORRECT**:
109+
```ruby
110+
def initialize(source, freeze_token: DEFAULT, signature_generator: nil, **options)
111+
@source = source
112+
@freeze_token = freeze_token
113+
@signature_generator = signature_generator
114+
# **options captures future parameters for forward compatibility
115+
end
116+
```
117+
118+
**WRONG**:
119+
```ruby
120+
def initialize(source, freeze_token: DEFAULT, signature_generator: nil)
121+
# This breaks when new parameters are added to the base class
122+
end
123+
```
124+
125+
**Why**: When `SmartMergerBase` adds new standard options (like `node_typing`, `regions`, etc.), all `FileAnalysis` classes automatically support them without code changes. Without `**options`, every FileAnalysis would need updating whenever a new option is added.
126+
127+
**Applies to**:
128+
- `FileAnalysis#initialize` in all gems
129+
- `SmartMerger#initialize` in all gems
130+
- Any method that accepts a variable set of options
131+
104132
### SmartMergerBase API
105133
- `merge` - Returns a **String** (the merged content)
106134
- `merge_result` - Returns a **MergeResult** object
@@ -124,9 +152,15 @@ When `replace_string_in_file` fails with "Could not find matching text":
124152

125153
## Loading Vendor Gems in Scripts
126154

127-
**IMPORTANT**: When writing standalone Ruby scripts to test vendor gems, you must use `bundler/setup` to properly load the gems.
155+
**IMPORTANT**: The approach depends on whether you're using the project's Gemfile or need standalone execution.
156+
157+
### For Scripts Using Project Gemfile (bundler/setup)
158+
159+
**Use bundler/setup when**:
160+
- The script runs within the project context
161+
- The Gemfile already specifies all needed gems with `path:` options
162+
- You want to use the exact versions locked in Gemfile.lock
128163

129-
**CORRECT** - Use bundler/setup:
130164
```ruby
131165
#!/usr/bin/env ruby
132166
# frozen_string_literal: true
@@ -137,22 +171,100 @@ require "prism/merge"
137171
# Now you can use Prism::Merge classes
138172
```
139173

174+
### For Standalone Scripts with Local Paths (bundler/inline)
175+
176+
**Use bundler/inline when**:
177+
- Creating standalone scripts in `bin/` that need specific gem paths
178+
- Testing with fixture gems or specific local paths
179+
- The script needs to specify its own dependencies independent of the project Gemfile
180+
- You need to load gems from vendor directories not in the main Gemfile
181+
182+
```ruby
183+
#!/usr/bin/env ruby
184+
# frozen_string_literal: true
185+
186+
require "bundler/inline"
187+
188+
gemfile do
189+
source "https://rubygems.org"
190+
gem "ast-merge", path: File.expand_path("..", __dir__)
191+
gem "tree_haver", path: File.expand_path("../vendor/tree_haver", __dir__)
192+
gem "markdown-merge", path: File.expand_path("../vendor/markdown-merge", __dir__)
193+
end
194+
195+
# Now gems are loaded and ready to use
196+
require "markdown/merge"
197+
```
198+
199+
**Why bundler/inline for standalone scripts:**
200+
1. The gemfile block creates an inline Gemfile with specified paths
201+
2. Bundler resolves dependencies and configures load paths
202+
3. Scripts become self-contained and portable
203+
4. No need to modify the project's main Gemfile
204+
205+
**Common pitfall with bundler/inline:**
206+
- If a gem in your inline gemfile has unresolved dependencies, bundler will try to fetch them
207+
- Solution: Only include gems you actually need, or ensure all transitive dependencies are available
208+
140209
**BROKEN** - These do NOT work:
141210
```ruby
142211
# This doesn't load the gem properly:
143212
require_relative "lib/prism/merge"
144213

145214
# This doesn't set up the load path:
146-
require "prism/merge" # without bundler/setup first
215+
require "prism/merge" # without bundler/setup or bundler/inline first
147216
```
148217

149-
The pattern `require "bundler/setup"` followed by `require "gem/name"` works because:
150-
1. `bundler/setup` configures the load path based on the Gemfile
151-
2. The vendor gems are specified in the Gemfile with `path:` option
152-
3. This allows standard `require` to find the gems
153-
154218
## Testing
155219

220+
### kettle-test RSpec Helpers
221+
222+
**IMPORTANT**: All spec files load `require "kettle/test/rspec"` which provides extensive RSpec helpers and configuration from the kettle-test gem. DO NOT recreate these helpers - they already exist.
223+
224+
**Environment Variable Helpers** (from `rspec-stubbed_env` gem):
225+
- `stub_env(hash)` - Temporarily set environment variables in a block
226+
- `hide_env(*keys)` - Temporarily hide environment variables
227+
228+
**Example usage**:
229+
They are not used with blocks, but can be used like this:
230+
```ruby
231+
before do
232+
stub_env("MY_ENV_VAR" => "Bla Blah Blu")
233+
end
234+
it "should see MY_ENV_VAR" do
235+
# code that reads ENV["MY_ENV_VAR"]
236+
end
237+
238+
# hide_env("HOME", "USER")
239+
# is used the same way, but hides the variable so it acts as it if isn't set at all.
240+
```
241+
242+
**Other Helpers** (loaded by kettle-test):
243+
- `block_is_expected` - Enhanced block expectations (from `rspec-block_is_expected`)
244+
- `capture` - Capture output during tests (from `silent_stream`)
245+
- Timecop integration for time manipulation
246+
247+
**Where these come from**:
248+
- External gems loaded by `kettle/test/external.rb` in the kettle-test gem
249+
- `rspec/stubbed_env` - Provides `stub_env` and `hide_env`
250+
- `rspec/block_is_expected` - Enhanced block expectations
251+
- `silent_stream` - Output suppression
252+
- `timecop/rspec` - Time travel for tests
253+
254+
**Other Helpers** (loaded by kettle-test):
255+
- `block_is_expected` - Enhanced block expectations (from `rspec-block_is_expected`)
256+
- `capture` - Capture output during tests (from `silent_stream`)
257+
- Timecop integration for time manipulation
258+
259+
**Where these come from**:
260+
- External gems loaded by `kettle/test/external.rb` in the kettle-test gem
261+
- `rspec/stubbed_env` - Provides `stub_env` and `hide_env`
262+
- `rspec/block_is_expected` - Enhanced block expectations
263+
- `silent_stream` - Output suppression
264+
- `timecop/rspec` - Time travel for tests
265+
266+
### Running Tests
267+
156268
Run tests from the appropriate directory:
157269
```bash
158270
# ast-merge tests
@@ -187,13 +299,15 @@ This runs tests with coverage instrumentation and generates detailed coverage re
187299

188300
**How `run_in_terminal` works**:
189301
- The tool sends commands to a **single persistent Copilot terminal**
302+
- Use `isBackground=false` for `run_in_terminal`. Sometimes it works, but if it fails/hangs, use the file redirection method, and then read back with `read_file` tool.
190303
- Commands run in sequence in the same terminal session
191304
- Environment variables and working directory persist between calls
192305
- The first command in a session either does not run at all, or runs before the shell initialization (direnv, motd, etc.) so it should always be a noop, like `true`.
193306

194307
**When things go wrong**:
195308
- If output shows only shell banner/motd without command results, the command most likely worked, but the tool has lost the ability to see terminal output. This happens FREQUENTLY.
196-
- EVERY TIME you do not see output, STOP and confirm output status with the user.
309+
- EVERY TIME you do not see output, STOP and confirm output status with the user, or switch immediately to file redirection, and read the file back with `read_file` tool.
310+
- **ALWAYS use project's `tmp/` directory for temporary files** - NEVER use `/tmp` or other system directories
197311
- Solution: Ask the user to share the output they see.
198312

199313
**Best practices**:

.github/workflows/locked_deps.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,8 @@ env:
2525
# because it would be redundant with the coverage workflow.
2626
# Also we can validate all output formats without breaking CodeCov,
2727
# since we aren't submitting these reports anywhere.
28-
K_SOUP_COV_MIN_BRANCH: 71
29-
K_SOUP_COV_MIN_LINE: 86
28+
K_SOUP_COV_MIN_BRANCH: 90
29+
K_SOUP_COV_MIN_LINE: 90
3030
K_SOUP_COV_MIN_HARD: false
3131
K_SOUP_COV_FORMATTERS: "html,xml,rcov,lcov,json,tty"
3232
K_SOUP_COV_DO: true

.gitlab-ci.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ variables:
2222
K_SOUP_COV_DEBUG: true
2323
K_SOUP_COV_DO: true
2424
K_SOUP_COV_HARD: true
25-
K_SOUP_COV_MIN_BRANCH: 74
25+
K_SOUP_COV_MIN_BRANCH: 90
2626
K_SOUP_COV_MIN_LINE: 90
2727
K_SOUP_COV_VERBOSE: true
2828
K_SOUP_COV_FORMATTERS: "tty"

Gemfile.lock

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ GEM
4444
thor (~> 1.0)
4545
concurrent-ruby (1.3.6)
4646
date (3.5.1)
47-
debug (1.11.0)
47+
debug (1.11.1)
4848
irb (~> 1.10)
4949
reline (>= 0.3.8)
5050
delegate (0.6.1)
@@ -140,15 +140,15 @@ GEM
140140
pdf-core (~> 0.10.0)
141141
ttfunk (~> 1.8)
142142
prettyprint (0.2.0)
143-
prism (1.6.0)
143+
prism (1.7.0)
144144
psych (5.3.1)
145145
date
146146
stringio
147147
public_suffix (7.0.0)
148148
racc (1.8.1)
149149
rainbow (3.1.1)
150150
rake (13.3.1)
151-
rbs (3.9.5)
151+
rbs (3.10.0)
152152
logger
153153
rdoc (6.17.0)
154154
erb

0 commit comments

Comments
 (0)