[NEP-20726] Dependency Pruning, Faraday 2.x and ruby compatibility #22
Conversation
SyntheticDave
left a comment
SyntheticDave
left a comment
There was a problem hiding this comment.
Looking good! A few notes
| @@ -0,0 +1,24 @@ | |||
| // For format details, see https://aka.ms/devcontainer.json. For config options, see the | |||
| // README at: https://github.com/devcontainers/templates/tree/main/src/ruby | |||
| { | |||
| @@ -0,0 +1,93 @@ | |||
| # Happi - AI Coding Instructions | |||
There was a problem hiding this comment.
🙌 We might want to get these instructions in our codebases that use Happi, unless there's a way to include these in the copilot instructions of dependent codebases.
| @@ -0,0 +1,26 @@ | |||
| name: Ruby | |||
There was a problem hiding this comment.
❗ Are we intending to replace the buildkite build with this action?
Looks like buildkite is still testing ruby 2.1.5
rbenv install -s 2.1.5
rbenv local 2.1.5Do we need to update or remove that?
There was a problem hiding this comment.
Oh, I don't see buildkite configured. Happy to remove and stick with this action. We can switch to GH actions on small test suites (<3 minutes).
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| ruby-version: ["3.2", "3.3", "3.4", "4.0"] |
There was a problem hiding this comment.
🌱 We could probably exclude 3.3 from testing as we're going from 3.2 to 3.4, and can likely assume if both of those build than so does 3.3.
Spec suite is small tho, so shouldn't be a problem to include.
|
|
||
| # You MUST use `adapter` method | ||
| conn = Faraday.new(...) do |f| | ||
| f.adapter AnyAdapter |
There was a problem hiding this comment.
📝 Looks like we're already using this pattern in JR
There was a problem hiding this comment.
Yeah, I ain't reading all that 😆
|
|
||
| ### ✅ IMPLEMENTED: Removed Custom JSON Middleware | ||
|
|
||
| Removed the undefined `JSON` constant middleware - Faraday's native JSON handling provides the same functionality. |
There was a problem hiding this comment.
📝 We're using similar middleware in JR. E.g. FaradayMiddleware::ParseJson.
Looks like we'll need to do the same there.
| @@ -1 +1 @@ | |||
| 3.2.0 | |||
| 3.4.8 | |||
There was a problem hiding this comment.
❗ This Happi version is being referenced in https://github.com/rdytech/bdf-service/pull/857, which is bumping the BDF Service to ruby 4.0. Will this cause issues?
There was a problem hiding this comment.
Since happi is already passing on ruby 4 via actions and passing on BDF, no issues I can see. This version is just the default for who wants to work with happi locally. I didn't move to 4 because the devcontainer is still a little unstable.
| end | ||
| end | ||
|
|
||
| describe '#connection' do |
There was a problem hiding this comment.
🔥 Great to add so much additional coverage!
| it 'returns a Logger writing to STDOUT' do | ||
| logger = client.default_logger | ||
| expect(logger).to be_a(Logger) | ||
| end |
There was a problem hiding this comment.
❓ Not sure this is testing exactly what the block claims?
No description provided.