Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define respond_to? on RecordRef to match method_missing behavior #608

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cgunther
Copy link
Contributor

@cgunther cgunther commented Mar 7, 2024

RecordRefs often include a name, which allows access via method_missing by calling record_ref.name, however if no referenced record is assigned, you'll still get a RecordRef instance, it just wont have any fields, so record_ref.name raised a NoMethodError, but if you checked for the field first via record_ref.respond_to?(:name), it'd be falsey regardless of whether the field existed or not.

Defining respond_to? in good practice alongside method_missing. Ideally, respond_to_missing? would be used instead, as that enables access to the method like record_ref.method(:name), however I kept with respond_to? to match CustomFieldList.

CustomRecordRef also uses method_missing, however there's no specs and I'm not familiar with that record, so I left it alone.

`RecordRef`s often include a name, which allows access via
`method_missing` by calling `record_ref.name`, however if no referenced
record is assigned, you'll still get a `RecordRef` instance, it just
wont have any fields, so `record_ref.name` raised a `NoMethodError`, but
if you checked for the field first via `record_ref.respond_to?(:name)`,
it'd be falsey regardless of whether the field existed or not.

Defining `respond_to?` in good practice alongside `method_missing`.
Ideally, `respond_to_missing?` would be used instead, as that enables
access to the method like `record_ref.method(:name)`, however I kept
with `respond_to?` to match `CustomFieldList`.

`CustomRecordRef` also uses `method_missing`, however there's no specs
and I'm not familiar with that record, so I left it alone.
@cgunther
Copy link
Contributor Author

cgunther commented Mar 7, 2024

CI is failing because of an upstream change in Nori, which can be fixed by #607

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.

1 participant