-
Notifications
You must be signed in to change notification settings - Fork 165
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
Changed all is_*? method names to *? #268
Conversation
Changed all is_*? method names to *?
This is my first open-source contribution ever, so please let me know if I've missed any steps. Thanks! |
@stephenandersondev thanks for submitting this I think this looks good, but I'll take a deeper look soon. @BuonOmo do you think we should alias all the methods to accept the is_*? form as well? It would be good for backwards compatibility. Also @stephenandersondev this is going to be part of our V3 release, so this needs to be merged into the |
@keithdoggett Just changed the branch to |
Thank you for doing it there!
@keithdoggett @stephenandersondev I thought a bit about it, and I think we can do some easier transition for users, that would even allow us to merge this in v2. I'm thinking about keep def is_simple?
warn "is_*? methods are deprecated, please use the *? counterpart, will be removed in v3" unless ENV["RGEO_SILENCE_DEPRECATION"]
simple?
end Of course, this might also be done using metaprogramming (for instance with What do you think about that idea, and @stephenandersondev could you implement it in this PR? 🙂 |
@BuonOmo Yes I can definitely implement this PR, I will have it ready shortly 👍 |
create alias methods
@BuonOmo I got the alias methods put in, have a couple of questions though:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephenandersondev thank you for the addition.
I didn't see a branch that referenced v2 so I assumed it was master since it's the current version? Let me know if otherwise and I can change it.
You're right, v2 is the current version, we're using the master branch.
I wasn't sure if we needed to add the alias methods to the tests? If so, let me know what you think that should look like and I'll get it in.
IMHo, that is not needed.
I've one last review concerning the geos capi, unfortunately we need alias for those methods as well. I've adressed which methods and where they should be aliased. I can do it and push to your branch later if you don't have more time 🙂 (I'll let you decide though, your branch, your ownership!)
After that I think we'll be ready to ship.
rb_define_method(geos_geometry_methods, "empty?", method_geometry_is_empty, 0); | ||
rb_define_method(geos_geometry_methods, "simple?", method_geometry_is_simple, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The geos methods do not implement Feature
base methods in v2 unfortunately.
Hence we also need to create the deprecation warning. You can do that one in ruby, in lib/rgeo/geos/capi_feature_classes.rb
:)
@@ -623,7 +623,7 @@ void rgeo_init_geos_geometry_collection(RGeo_Globals* globals) | |||
geos_multi_line_string_methods = rb_define_module_under(globals->geos_module, "CAPIMultiLineStringMethods"); | |||
rb_define_method(geos_multi_line_string_methods, "geometry_type", method_multi_line_string_geometry_type, 0); | |||
rb_define_method(geos_multi_line_string_methods, "length", method_multi_line_string_length, 0); | |||
rb_define_method(geos_multi_line_string_methods, "is_closed?", method_multi_line_string_is_closed, 0); | |||
rb_define_method(geos_multi_line_string_methods, "closed?", method_multi_line_string_is_closed, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here unfortunately
rb_define_method(geos_line_string_methods, "closed?", method_line_string_is_closed, 0); | ||
rb_define_method(geos_line_string_methods, "ring?", method_line_string_is_ring, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and there
I think this will work?
So sorry about that @BuonOmo. I'm admittedly very new to RGeo and this is my first time ever looking at C code 😅 I think I got the aliasing right? Although, you may mean to alias do the aliasing on the ruby side which is where I would add the warning back in at? If this is wrong (which I feel it is) would you be able to do one as an example? I would be happy to do the rest 🙂 Sorry again, still learning this stuff. |
@stephenandersondev no problem, I'll give you some pointers. Let's try with the We first need to know where the method is implemented. The line below gives us this information: rb_define_method(geos_geometry_methods, "empty?", method_geometry_is_empty, 0); We can see it is defined under geos_geometry_methods = rb_define_module_under(globals->geos_module, "CAPIGeometryMethods"); This module can be found when searching the project, and you'll most likely end up in module RGeo
module Geos
module CAPIGeometryMethods # :nodoc:
include Feature::Instance
def is_empty?
warn "support..."
empty?
end
... Is that clear enough? If not, I'll just takeover this last bit and you'll see my commit as an example 🙂 |
Make suggested corrections on aliasing
@BuonOmo Thank you so much for taking the time to clarify this for me, I learned something new because of it 😄 I believe this last commit I pushed does the trick. Please let me know if otherwise. Thanks again for your help! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@keithdoggett do you want to process to one last review? |
Oh, my bad, rubocop is not passing anymore, @stephenandersondev the trick is to disable the |
Disable Naming/PredicateName cop
So sorry about that 🤦🏻 Just committed the disabling of it. Thank you so much for your patience and guidance on this! |
@stephenandersondev sorry I should have been a bit clearer.... I think that this should be inlined: def is_simple? # rubocop:disable Naming/PredicateName This will ensure that when getting rid of every |
Make magic comments inline
@BuonOmo Oops sorry about that, they're inline now 👍🏻 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work @stephenandersondev and @BuonOmo! These look good for the most part. My only suggestion is that we don't modify the History.md
file because that's supposed to reflect historical changes that happened and probably shouldn't be modified only appended to.
History.md
Outdated
* The class shuffle in 0.3.14 broke RGeo::Geos.is_geos? and similar. Fixed. | ||
* The class shuffle in 0.3.14 broke RGeo::Geos.geos? and similar. Fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we should change this since it's describing a past change.
History.md
Outdated
LineString#is_simple? (which should now allow LinearRing to work). | ||
LineString#simple? (which should now allow LinearRing to work). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we should change this since it's describing a past change.
revert History.md
@keithdoggett Ahhh that's a great point. I just reverted them back. Thanks for the head up! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks @stephenandersondev! LGTM
@BuonOmo should be good to merge as is unless you want to squash the commits first.
Merged! Thanks again @stephenandersondev :) |
Thank you both! I look forward to working with you again :) |
well if you see something on which you could work on the roadmap, go ahead 🙂 |
Summary
Changed all is_*? method names to *? on the following:
Closes #264