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

Feature/rails 6 #11

Merged
merged 5 commits into from
Jul 1, 2020
Merged

Feature/rails 6 #11

merged 5 commits into from
Jul 1, 2020

Conversation

tagliala
Copy link
Member

@tagliala tagliala commented Dec 3, 2019

Supports both module_parent and module_parents

Superseeds #10

@vjt
Copy link
Contributor

vjt commented Dec 3, 2019

Thank you

@tagliala
Copy link
Member Author

tagliala commented Dec 3, 2019

you're welcome!

@vjt I'm waiting for an all-green test matrix on travis ci before removing the draft status

@tagliala tagliala marked this pull request as ready for review December 3, 2019 17:51
@tagliala
Copy link
Member Author

tagliala commented Dec 3, 2019

All green

Copy link
Contributor

@vjt vjt left a comment

Choose a reason for hiding this comment

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

the respond_to? :module_parent are ugly, should be replaced with our polyfill implementation that just calls parent.

@@ -80,7 +80,12 @@ def method_missing(meth, *args, &block)
if klass.respond_to?(meth)

method = klass.method(meth)
dsl_method = method.owner.parents.include?(Hawk::Model)
dsl_method =
if method.owner.respond_to?(:module_parents)
Copy link
Contributor

Choose a reason for hiding this comment

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

not nice, can we define module_parent as parent if not
defined, and change these points just by referencing module_parent instead of parent?

thx

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry @vjt, I'm not sure I've got the suggestion

owner may be Class or Module objects that could have not been defined by Hawk. parents and module_parents are monkey patch added by Active Support on top of Ruby's Module.

Hawk's specs only cover cases in which the method owner is Hawk::Model::Querying::ClassMethods and Hawk::Model is always present in the test cases. When running tests on a monolithic Rails application using Hawk, this method will check for owners whose the only parent is Object

Copy link
Contributor

Choose a reason for hiding this comment

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

The suggestion (if applicable) would be us to define module_parent if not defined yet by Rails. Would that work?

@lleirborras
Copy link
Member

@tagliala how r we doing with this? and why is there another PR for the same?

@tagliala
Copy link
Member Author

tagliala commented Jul 1, 2020

how r we doing with this?

we need to decide how to implement the module_parents check

and why is there another PR for the same?

This supports both versions of Active Support, without breaking for Rails < 6

Check the travis builds:

@vjt
Copy link
Contributor

vjt commented Jul 1, 2020

how r we doing with this?
we need to decide how to implement the module_parents check

I can be OK with the code smell I see if this blocks you @lleirborras, it can be refactored at a later stage

@vjt vjt mentioned this pull request Jul 1, 2020
@@ -166,7 +166,7 @@ def to_s
Caster.new(:float, -> (value) { Float(value) }),
Caster.new(:datetime, -> (value) { Time.parse(value) }),
Caster.new(:date, -> (value) { Date.parse(value) }),
Caster.new(:bignum, -> (value) { BigDecimal.new(value) }),
Caster.new(:bignum, -> (value) { BigDecimal(value) }),
Copy link
Member Author

@tagliala tagliala Jul 1, 2020

Choose a reason for hiding this comment

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

I've checked that changing from BigDecimal.new() to BigDecimal(value) also works on Ruby 1.9 (excluded from the travis jobs matrix)

.travis.yml Show resolved Hide resolved
@tagliala
Copy link
Member Author

tagliala commented Jul 1, 2020

Please wait

@tagliala tagliala marked this pull request as draft July 1, 2020 21:15
@vjt
Copy link
Contributor

vjt commented Jul 1, 2020

- Fixnum has been deprecated in favor of Integer
- BigDecimal.new has been deprecated in favor of Bigdecimal()
By testing against unreleased versions of Ruby and Rails, we can spot 
incompatibilities, deprecations, and warnings ahead of time
@tagliala tagliala marked this pull request as ready for review July 1, 2020 21:37
@tagliala
Copy link
Member Author

tagliala commented Jul 1, 2020

All green: https://travis-ci.org/github/ifad/hawk/builds/704097018

Copy link
Contributor

@vjt vjt left a comment

Choose a reason for hiding this comment

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

OK to go for now, we can review at a later stage the module_parent comment in #12.

@lleirborras
Copy link
Member

how r we doing with this?
we need to decide how to implement the module_parents check

I can be OK with the code smell I see if this blocks you @lleirborras, it can be refactored at a later stage

It does not block me, I can live with deprecation warnings

@tagliala
Copy link
Member Author

tagliala commented Jul 1, 2020

I do not have merge permissions here, feel free to merge at your convenience

@vjt vjt merged commit 8555c53 into ifad:master Jul 1, 2020
@vjt
Copy link
Contributor

vjt commented Jul 1, 2020

Merged and access rights fixed, for future 🥳 and 🍻 .

Thanks!

@tagliala tagliala deleted the feature/rails-6 branch July 2, 2020 08:35
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