Skip to content

Added plugin registration for custom validators - #16

Draft
HassanAkbar wants to merge 6 commits into
mainfrom
feat/plugin-base
Draft

Added plugin registration for custom validators#16
HassanAkbar wants to merge 6 commits into
mainfrom
feat/plugin-base

Conversation

@HassanAkbar

Copy link
Copy Markdown
Member

Wires the existing plugin registry into Schematron validation, so a registered rule actually runs.

The registry already existed but nothing read it. Registering a plugin did nothing
observable. Profile now composes its built-in rules with Dcc::Plugin.all(:validators),
and Dcc.load_plugins loads plugin gems by name.

Known and accepted:

  • Only validators are wired. Converters and CLI commands stay registered-but-unread.
    Converter dispatch is a hardcoded case. CLI plugins need exe/dcc changes. Both
    are their own PR.
  • register_validator rejects an instance where a class is expected. Profile calls
    rule_class.new, so an instance would fail deep inside validation.
  • Dcc::Plugin.register stays unguarded. It is the raw primitive. The documented path
    is include Dcc::Plugin::Base.
  • DEFAULT_RULES is a private constant. Tests assert through Profile#rules.
  • Two specs look vacuous. Each is the negative control for a positive example above it.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Wires the existing Dcc::Plugin registry into Schematron validation and adds a Dcc.load_plugins helper so validator plugins can be discovered, loaded, and executed as part of the validation profile.

Changes:

  • Compose Schematron Profile rules from built-ins plus Dcc::Plugin.all(:validators) and freeze the exposed rule list.
  • Introduce Dcc::Plugin::Base with register_validator (including guardrails) and extend plugin-related specs/fixtures.
  • Add Dcc.load_plugins and a dedicated Dcc::PluginError for plugin load failures.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
spec/fixtures/plugins/dcc/sample_plugin.rb Adds a fixture “plugin gem” entry file that registers a validator.
spec/dcc/validate/schematron_spec.rb Adds specs asserting plugin validators are appended and executed during real Schematron runs.
spec/dcc/plugin/base_spec.rb Adds unit tests for Dcc::Plugin::Base validator registration and input validation.
spec/dcc/plugin_spec.rb Ensures plugin registry reset happens after specs as well as before.
spec/dcc_spec.rb Adds .load_plugins specs and sets up fixture plugin loadability.
lib/dcc/validate/schematron/profile.rb Refactors built-in rule list into a private constant and appends registered plugin validators.
lib/dcc/plugin/base.rb Adds the plugin “declarative” API (include ...; register_validator ...) with guardrails.
lib/dcc/plugin.rb Updates docs and autoloads Dcc::Plugin::Base.
lib/dcc/error.rb Adds Dcc::PluginError for plugin loading failures.
lib/dcc.rb Implements Dcc.load_plugins gem-name-to-require-path mapping and error wrapping.

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

Comment thread lib/dcc.rb
Comment thread spec/fixtures/plugins/dcc/sample_plugin.rb Outdated

Copilot AI left a comment

Copy link
Copy Markdown

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 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (2)

lib/dcc.rb:99

  • load_plugins rescues any LoadError, including ones raised from inside the plugin’s entry file (e.g., a missing dependency the plugin itself requires). That can misreport the failure as “could not load plugin …” even though the entry file was found, and it hides the original error type/backtrace for the real missing file. Consider only wrapping the LoadError when it corresponds to the entry path you attempted, and re-raise otherwise.
      rescue ::LoadError => e
        raise PluginError, "could not load plugin #{name}: tried " \
                           "require #{path.inspect} (#{e.message})"
      end

spec/dcc_spec.rb:6

  • This spec file permanently prepends the plugin fixtures directory to $LOAD_PATH, which is global process state and can affect requires in unrelated specs (especially since unshift changes resolution precedence). Consider cleaning it up to keep the test environment isolated.
$LOAD_PATH.unshift(fixtures_path("plugins"))

Copilot AI left a comment

Copy link
Copy Markdown

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 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (1)

spec/dcc_spec.rb:7

  • Modifying $LOAD_PATH at file load time affects the entire spec process and can make unrelated specs accidentally resolve require "dcc/..." to fixtures when the real file is missing. This is especially risky because the fixture tree mirrors real dcc/ paths. Consider scoping this $LOAD_PATH change to the .load_plugins examples (e.g., an around hook that pushes before and deletes after) so the rest of the suite can’t be influenced by plugin fixtures.
# Appended, not prepended: the fixture tree mirrors real `dcc/` paths, so
# giving it precedence would let a fixture shadow the gem's own files.
$LOAD_PATH.push(fixtures_path("plugins"))

@HassanAkbar
HassanAkbar marked this pull request as ready for review August 11, 2026 15:30
@HassanAkbar
HassanAkbar marked this pull request as draft August 11, 2026 15:34
@HassanAkbar
HassanAkbar requested a lite review from Copilot August 13, 2026 09:43

Copilot AI left a comment

Copy link
Copy Markdown

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 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (2)

lib/dcc/plugin/base.rb:38

  • register_validator currently accepts anonymous classes (e.g., Class.new) as long as they define #check_on. If such a rule ever emits an issue via Dcc::Validate::Schematron::Rule#issue, Rule#code will call self.class.name.split(...) and crash when name is nil. Consider either (a) rejecting unnamed rule classes in rule_class! with a clear error, or (b) updating the rule code generator to handle anonymous classes safely.
      def self.rule_class!(entry)
        return entry if rule_class?(entry)

        raise ::ArgumentError,
              "expected a rule class responding to #check_on, " \
              "got #{describe(entry)}"
      end

      # @param entry [Object]
      # @return [Boolean]
      def self.rule_class?(entry)
        entry.is_a?(::Class) && entry.method_defined?(:check_on)
      end

lib/dcc/validate/schematron/profile.rb:33

  • DEFAULT_RULES is missing Rules::AdministrativeDataCompleteness. That rule is autoloaded in lib/dcc/validate/schematron/rules.rb but never executed by the Schematron profile, and the surrounding docs refer to 14 PTB rules. If this rule is intended to be part of the built-in profile, it should be added to the default execution list so it can actually run.
          Rules::LanguageCodeDedup,
          Rules::XmlListSpacing,
        ].freeze

Copilot AI left a comment

Copy link
Copy Markdown

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 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (1)

lib/dcc/plugin/base.rb:38

  • rule_class? currently uses method_defined?(:check_on), which returns true for protected methods and does not validate that check_on can be called with the 1 argument that Profile#call passes (rule_class.new.check_on(dcc)). This can allow invalid validators to register successfully but then raise at runtime during validation. Consider requiring a public check_on and verifying that it accepts at least one argument (and no more than one required argument).
      def self.rule_class?(entry)
        entry.is_a?(::Class) && entry.method_defined?(:check_on)
      end

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.

2 participants