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

Improve reification of STI classes #1333

Merged
merged 6 commits into from
Jul 31, 2021
Merged

Conversation

hschne
Copy link
Contributor

@hschne hschne commented Jul 23, 2021

Checklist

  • Wrote [good commit messages][1].
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new
    code introduces user-observable changes.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.

This fixes #1332.

As discussed, two commits were created, one with failing test cases and one to fix them. Some details about this solution:

I opted to add a new test model, rather than an modify an existing one (e.g. Animal) to better represent the specific edge case described in the issue.

Futhermore, as sti_class_for was only added recently (Rails 6.1), reification falls back to calling the private method find_sti_class. I could not find another way to keep tests for older versions of Rails passing.

I left the commits unsquashed for transparency, feel free to squash when merging.

@hschne hschne changed the title Improve reifieing STI classes Improve reification of STI classes Jul 23, 2021
@jaredbeck jaredbeck requested a review from aried3r July 26, 2021 16:19
Copy link
Member

@jaredbeck jaredbeck left a comment

Choose a reason for hiding this comment

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

This is an interesting STI feature that I had not heard of before. As of 6.1, it is a feature of Rails' public API, so I'm happy to support it.

Your description of the issue was excellent. Please boil that down into a few paragraphs, and add them to the comments above #version_reification_class.

@hschne
Copy link
Contributor Author

hschne commented Jul 31, 2021

I updated the comments, let me know what you think :)

@hschne hschne requested a review from jaredbeck July 31, 2021 16:24
@jaredbeck jaredbeck merged commit 655e78a into paper-trail-gem:master Jul 31, 2021
@jaredbeck
Copy link
Member

Thanks Hans, nice work!

@jaredbeck
Copy link
Member

Released in 12.1.0

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.

Improve fetching reified class names for STI models
2 participants