Skip to content

Conversation

@Aelphaeis
Copy link
Contributor

@Aelphaeis Aelphaeis commented Oct 20, 2021

Presently we do not have support for rails 6.1.4.1.

The objective of this pull request is to fix tests and ensure that the polymorphic integer type works with rails 6.1.4.1 as well as update CI to test this functionality.

Several tests are broken because rails/rails#40969 & rails/rails#40943 were fixed/closed by rails/rails@b52b804.
The net effect that the original replace_keys has been updated to have a new argument called force

I've made some changes to better support continuous integration.

fixes #38

@Aelphaeis Aelphaeis self-assigned this Oct 25, 2021
@Aelphaeis
Copy link
Contributor Author

Several tests are broken because rails/rails#40969 & rails/rails#40943 were fixed/closed by rails/rails@b52b804.

The net effect that the original replace_keys has been updated to have a new argument called force

The stack trace comes from the BelongsToPolymorphicAssociation.replace_keys and looks as follows:

  1) PolymorphicIntegerType when the source is nil should have the no id/type for the source
     Failure/Error:
       private def replace_keys(record)
         super
         unless record.nil?
           owner[reflection.foreign_type] = record.class.base_class
         end

     ArgumentError:
       wrong number of arguments (given 2, expected 1)
     # ./lib/polymorphic_integer_type/belongs_to_polymorphic_association_extension.rb:4:in `replace_keys'
     # ./lib/polymorphic_integer_type/module_generator.rb:24:in `block (2 levels) in generate_and_include'

To fix while preserving existing behaviour we should set the force variable to true; however, this has the negative side effect of re-introducing the bugs this fixes. Will investigate other solutions.

@Aelphaeis
Copy link
Contributor Author

Aelphaeis commented Oct 25, 2021

I've made some changes to better support continuous integration.

Previously bundler cache was set to true. This had caused CI to run the following command

  /opt/hostedtoolcache/Ruby/2.6.8/x64/bin/bundle config --local deployment true

When you used bundle config to set the deployment to true the gemfile must match the gemfile.lock.

This meant that if you changed a dependency in the gemspec you were forced to run bundle install --gemfile path/to/gemfile on each of the gemfiles and check in the resulting lock files.

I felt that this was unintuitive and a lot of work, I also felt that the setting deployment to true was unnecessary and decided to do the bundle install manually. This results in us no longer needing to update the lock files. Since all of the required libraries are publicly available in ruby gems and on github I think this is safe.

This allowed us to delete the lock files in d9e7c8d.

@Aelphaeis Aelphaeis changed the title Support for rails below Support for Rails 6.1.4.1 and onward (Until 7) Oct 25, 2021
@Aelphaeis
Copy link
Contributor Author

Aelphaeis commented Oct 25, 2021

rails/rails@ccb13cb renames association to reflection. This causes our patch to fail. I've updated it as a result.

@Aelphaeis Aelphaeis marked this pull request as ready for review October 25, 2021 18:24
@Aelphaeis Aelphaeis requested review from a team as code owners October 25, 2021 18:24
@Aelphaeis Aelphaeis changed the title Support for Rails 6.1.4.1 and onward (Until 7) Support for Rails 6.1.4.1 and onward (Until 7) + Supporting CI Changes. Oct 25, 2021
Copy link
Contributor

@rhemsing rhemsing left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Joseph and I chatted through the downsides of not including lock files. I think we're probably fine because of the nature of this gem -- it's very specific functionality that is covered by CI.

runs-on: ubuntu-latest

strategy:
fail-fast: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Note for other reviewers - Joseph and I chatted about this -- this will ensure that if specs for one Rails version fail, it won't auto-fail the other specs for other versions

@Aelphaeis Aelphaeis merged commit 01be377 into master Oct 26, 2021
@Aelphaeis Aelphaeis deleted the rails-6.1.4.1 branch November 3, 2021 21:10
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.

Rails 6.1 Support

6 participants