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

fix: avoid ruby warning for too many args #33

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

Conversation

BuonOmo
Copy link

@BuonOmo BuonOmo commented Sep 30, 2022

When running RGeo test we always get this warning:

/Users/u.buonomo/.rbenv/versions/3.2.0-dev/lib/ruby/gems/3.2.0+2/gems/ffi-geos-2.3.1/lib/ffi-geos.rb:1255: warning: too many arguments for format string

Which comes from the fact that we run out tests with the ruby_verbose flag (seems like a rake default config). I'd like to keep using this flag while not have this error message.

The solution seems pretty reasonable to me, and TBH I think we might go further and only print args[0] because I did not see any time args[1] containing anything within rgeo's test suite at least

@dark-panda
Copy link
Owner

This code might not be needed at all now. Perhaps I can just remove it altogether if that would be preferable?

@BuonOmo
Copy link
Author

BuonOmo commented Oct 24, 2022

@dark-panda do you mean removing the error altogether? This would be a huge breaking change for sure.. Did I miss something?

@dark-panda
Copy link
Owner

No, just removing the ternary and formatting code, so the result would just be

def default_error_handler(arg)
  raise Geos::GEOSException, arg
end

Since I don't see any location in GEOS that uses multiple arguments in this manner. I've looked around libgeos and can't find any instances where the formatting path was used, but I can check that again.

@BuonOmo
Copy link
Author

BuonOmo commented Oct 25, 2022

Yes I'd definitely drop that. In RGeo test suite we always have strictly one argument when this method is called.

Seeing in capi/geos_ts_c.cpp, calls to ERROR_MESSAGE are always with a single string (same for NOTICE_MESSAGE)

@BuonOmo
Copy link
Author

BuonOmo commented Apr 2, 2023

Hey @dark-panda, shouldn't we go forward with this PR?

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.

2 participants