Skip to content

Conversation

@addersuk
Copy link
Member

  • Introduced RBS type validation in CI workflows.
  • Added RBS validation task in Rakefile.
  • Created RBS type signatures for various modules and classes.
  • Updated gemspec to include RBS as a development dependency.

This pull request introduces RBS (Ruby Signature) type checking to the project, adds type signature files for key classes and modules, and updates the CI workflow to validate RBS types during testing and publishing. The changes improve type safety, documentation, and reliability of the codebase.

RBS Type Checking Integration:

  • Added a new Rake task (:rbs) to validate RBS type signatures using the rbs gem, and included rbs as a development dependency in the gemspec. [1] [2]
  • Updated GitHub Actions workflows (ruby.yml and gem-publish.yml) to run RBS validation as part of the CI process. [1] [2]

Type Signature Files:

  • Added RBS files for core classes and modules, including Twiglet::Logger, Twiglet::Formatter, Twiglet::Validator, Twiglet::Message, and Twiglet::ErrorSerialiser, as well as for utility modules and external dependencies (hash_extensions.rbs, json-schema.rbs). [1] [2] [3] [4] [5] [6] [7]
  • Added a manifest.yaml listing RBS dependencies for development and standard library usage.
  • Added an RBS file for the Twiglet::VERSION constant.

- Introduced RBS type validation in CI workflows.
- Added RBS validation task in Rakefile.
- Created RBS type signatures for various modules and classes.
- Updated gemspec to include RBS as a development dependency.
Copilot AI review requested due to automatic review settings December 30, 2025 13:36
@addersuk addersuk requested a review from a team as a code owner December 30, 2025 13:36
Copy link
Contributor

Copilot AI left a 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 pull request introduces RBS (Ruby Signature) type checking to enhance type safety and documentation. However, the PR contains critical issues in the gem-publish.yml workflow that appear to be unintended changes unrelated to RBS integration.

Key Changes:

  • Added RBS type signature files for core classes (Logger, Formatter, Validator, Message, ErrorSerialiser) and dependencies
  • Integrated RBS validation into CI workflows and Rakefile
  • Added rbs as a development dependency in gemspec

Reviewed changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
twiglet.gemspec Added rbs as a development dependency
sig/twiglet/version.rbs Type signature for VERSION constant
sig/twiglet/validator.rbs Type signatures for Validator class
sig/twiglet/message.rbs Type signatures for Message class
sig/twiglet/logger.rbs Type signatures for Logger class methods and initialization
sig/twiglet/formatter.rbs Type signatures for Formatter class
sig/twiglet/error_serialiser.rbs Type signatures for ErrorSerialiser class
sig/manifest.yaml Lists RBS dependencies for standard library usage
sig/json-schema.rbs Type stubs for json-schema gem
sig/hash_extensions.rbs Type signatures for HashExtensions module
Rakefile Added rake task to validate RBS type signatures
.github/workflows/ruby.yml Added RBS validation step to CI
.github/workflows/gem-publish.yml Added RBS validation; contains critical breaking changes to build job

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

**untyped args
) -> void

def configure_validation_error_response: () { (JSON::Schema::ValidationError) -> void } -> void
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type for configure_validation_error_response should be void rather than having a block that returns void. The method itself doesn't return a meaningful value, it just sets the custom error handler. The correct signature should be def configure_validation_error_response: () { (JSON::Schema::ValidationError) -> untyped } -> void.

Suggested change
def configure_validation_error_response: () { (JSON::Schema::ValidationError) -> void } -> void
def configure_validation_error_response: () { (JSON::Schema::ValidationError) -> untyped } -> void

Copilot uses AI. Check for mistakes.

build:
name: Build and Publish
runs-on: ubuntu-latest
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build job is missing the needs: test dependency that was removed in this PR. This means the build job will run in parallel with tests rather than waiting for tests to pass first. This could result in publishing a gem even if tests or RBS validation fail.

Suggested change
runs-on: ubuntu-latest
runs-on: ubuntu-latest
needs: test

Copilot uses AI. Check for mistakes.
- uses: actions/checkout@v5
- name: Set up Ruby
uses: ruby/setup-ruby@v1
with:
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ruby-version: .ruby-version configuration was removed from the setup-ruby action. This means the build job will use a default Ruby version rather than the project's specified version, which could lead to compatibility issues during gem building and publishing.

Suggested change
with:
with:
ruby-version: .ruby-version

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,11 @@
module Twiglet
class Validator
attr_accessor custom_error_handler: ^(JSON::Schema::ValidationError) -> void
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type signature for custom_error_handler is incorrect. Looking at the implementation in lib/twiglet/validator.rb, the custom error handler is initialized as a lambda that takes an exception and either raises it or performs custom logic, but it doesn't have a void return type constraint. The handler can have any return value. The correct signature should be ^(JSON::Schema::ValidationError) -> untyped instead of ^(JSON::Schema::ValidationError) -> void.

Suggested change
attr_accessor custom_error_handler: ^(JSON::Schema::ValidationError) -> void
attr_accessor custom_error_handler: ^(JSON::Schema::ValidationError) -> untyped

Copilot uses AI. Check for mistakes.
@addersuk
Copy link
Member Author

/dobby version minor

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 18 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- uses: actions/checkout@v6
- name: Set up Ruby
uses: ruby/setup-ruby@v1
with:
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ruby-version: .ruby-version line has been removed from the build job setup. This means the build job will now use the default Ruby version provided by the runner instead of the project's specified Ruby version in .ruby-version. This could lead to building the gem with a different Ruby version than intended. Consider restoring this configuration or explicitly specifying the Ruby version.

Suggested change
with:
with:
ruby-version: .ruby-version

Copilot uses AI. Check for mistakes.
Comment on lines +2 to +3
class Message < Hash[Symbol, untyped]
def initialize: (String | Hash[untyped, untyped] | nil msg) -> void
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Message class inherits from Hash[Symbol, untyped] in the RBS signature, but in the actual implementation (lib/twiglet/message.rb), it inherits from just Hash without type parameters. Additionally, the initialize method can accept nil and will call super for the nil case, which doesn't match the documented behavior in the signature. The signature should more accurately reflect that when msg is nil, the behavior delegates to Hash's default initialize.

Suggested change
class Message < Hash[Symbol, untyped]
def initialize: (String | Hash[untyped, untyped] | nil msg) -> void
class Message < Hash[untyped, untyped]
def initialize: () -> void
| (String | Hash[untyped, untyped] | nil msg) -> void

Copilot uses AI. Check for mistakes.
Comment on lines +45 to +54
- name: Publish to RubyGems
run: |
mkdir -p $HOME/.gem
touch $HOME/.gem/credentials
chmod 0600 $HOME/.gem/credentials
printf -- "---\n:rubygems_api_key: ${GEM_HOST_API_KEY}\n" > $HOME/.gem/credentials
gem build *.gemspec
gem push *.gem
env:
GEM_HOST_API_KEY: ${{secrets.RUBYGEMS_API_KEY}}
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The publish step uses a manual gem build and push approach instead of using the official rubygems/release-gem action that was presumably used before. While this can work, the manual approach requires managing credentials directly and is more error-prone. Additionally, building without specifying a Ruby version (due to the removal of the ruby-version configuration) could lead to inconsistencies. Consider using the official action or ensuring the Ruby version is explicitly set.

Copilot uses AI. Check for mistakes.
Comment on lines +23 to +26
def debug: (?String | Exception | nil message_or_error) ?{ () -> (String | Hash[untyped, untyped]) } -> true
def info: (?String | Exception | nil message_or_error) ?{ () -> (String | Hash[untyped, untyped]) } -> true
def warn: (?String | Exception | nil message_or_error) ?{ () -> (String | Hash[untyped, untyped]) } -> true
def error: (?String | Exception | nil message_or_error, ?Exception? error) ?{ () -> (String | Hash[untyped, untyped]) } -> true
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type for the error method signature is inconsistent with the actual implementation. Looking at the implementation in lib/twiglet/logger.rb, the error method calls super(message, &) which returns the result from the parent ::Logger#error method. The parent Logger class's error method can return different values depending on the logger's state, not just true. The same issue applies to debug, info, and warn methods.

Suggested change
def debug: (?String | Exception | nil message_or_error) ?{ () -> (String | Hash[untyped, untyped]) } -> true
def info: (?String | Exception | nil message_or_error) ?{ () -> (String | Hash[untyped, untyped]) } -> true
def warn: (?String | Exception | nil message_or_error) ?{ () -> (String | Hash[untyped, untyped]) } -> true
def error: (?String | Exception | nil message_or_error, ?Exception? error) ?{ () -> (String | Hash[untyped, untyped]) } -> true
def debug: (?String | Exception | nil message_or_error) ?{ () -> (String | Hash[untyped, untyped]) } -> untyped
def info: (?String | Exception | nil message_or_error) ?{ () -> (String | Hash[untyped, untyped]) } -> untyped
def warn: (?String | Exception | nil message_or_error) ?{ () -> (String | Hash[untyped, untyped]) } -> untyped
def error: (?String | Exception | nil message_or_error, ?Exception? error) ?{ () -> (String | Hash[untyped, untyped]) } -> untyped

Copilot uses AI. Check for mistakes.
gem.add_development_dependency 'rake'
gem.add_development_dependency 'simplecov', '0.17.1'
gem.add_development_dependency 'rbs'
gem.add_development_dependency 'simplecov'
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version constraint for SimpleCov has been removed. The previous version was pinned to 0.17.1. If this unpinning is intentional, it's acceptable, but be aware that this may introduce breaking changes if SimpleCov has released versions with incompatible APIs since 0.17.1. Consider verifying that the latest SimpleCov version is compatible with the test suite.

Suggested change
gem.add_development_dependency 'simplecov'
gem.add_development_dependency 'simplecov', '0.17.1'

Copilot uses AI. Check for mistakes.

gem.require_paths = ['lib']
gem.required_ruby_version = ['>= 3.1 ', '< 3.5']
gem.required_ruby_version = ['>= 3.2 ', '< 3.5']
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The minimum Ruby version requirement has been changed from >= 3.1 to >= 3.2. This is a breaking change that will prevent users on Ruby 3.1 from using this version of the gem. Ensure this change is documented in the changelog and that the version bump to 3.15.0 follows semantic versioning guidelines (this should likely be a major version bump if Ruby 3.1 support is being dropped).

Suggested change
gem.required_ruby_version = ['>= 3.2 ', '< 3.5']
gem.required_ruby_version = ['>= 3.1', '< 3.5']

Copilot uses AI. Check for mistakes.
@addersuk addersuk merged commit edec111 into master Dec 30, 2025
14 checks passed
@addersuk addersuk deleted the rbs-types branch December 30, 2025 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants