search

googleads / google-api-ads-ruby

Ad Manager SOAP API Client Libraries for Ruby
297 stars 227 forks source link

DfpApi::SUGGESTED_PAGE_LIMIT is initialized twice #148

Closed radarek closed 6 years ago

radarek commented 6 years ago

google-dfp-api 1.2.0 gem produces warning:

$ ruby -e 'require "dfp_api"'
/Users/radarek/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/google-dfp-api-1.2.0/lib/dfp_api/pql_statement_utils.rb:25: warning: already initialized constant DfpApi::SUGGESTED_PAGE_LIMIT
/Users/radarek/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/google-dfp-api-1.2.0/lib/dfp_api/dfp_api_statement.rb:23: warning: previous definition of SUGGESTED_PAGE_LIMIT was here

SUGGESTED_PAGE_LIMIT is created in two places: https://github.com/googleads/google-api-ads-ruby/blob/a4d80d50ae96c228ef2ab82a044abb736b17b248/dfp_api/lib/dfp_api/pql_statement_utils.rb#L25 https://github.com/googleads/google-api-ads-ruby/blob/a4d80d50ae96c228ef2ab82a044abb736b17b248/dfp_api/lib/dfp_api/dfp_api_statement.rb#L23

Comments suggest that there should be two different constants for StatementBuilder and FilterStatement classes so it would be natural to move it there. On the other hand examples directory uses DfpApi::SUGGESTED_PAGE_LIMIT constant so maybe there should be only one constant. You have to decide.

donovanfm commented 6 years ago

Thanks for letting us know, radarek. We will fix this soon.

donovanfm commented 6 years ago

This is fixed as of version 1.2.1. I moved the SUGGESTED_PAGE_LIMIT constant in StatementBuilder to the class itself, which is a more appropriate location. That is, there is no real need to access this constant outside of StatementBuilder like there was with FilterStatement since with StatementBuilder you can access @limit directly.