payprop / html-googlemaps-v3

Fork of HTML::GoogleMaps to fix issues
2 stars 1 forks source link

Test with different GeoCoders #13

Closed nigelhorne closed 7 years ago

nigelhorne commented 7 years ago

Test the workaround for Github issue #12 Document the geocoder option Fix typo that z-axis should have read z-index Only load G:C:G when we're going to use it Improved CI testing

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.2%) to 97.619% when pulling 84b861bcc177ad35234fc17035cd69f49ea823cd on nigelhorne:master into d77115a1835f1ee681319bd7598c6e51fff0787c on Humanstate:master.

nigelhorne commented 7 years ago

That's the simple way I prefer to work. You can ignore it, I know you work in a different way.

-Nigel

On 21/12/16 09:44, Lee J wrote:

@leejo commented on this pull request.


In .travis.yml https://github.com/Humanstate/html-googlemaps-v3/pull/13#pullrequestreview-13977165:

script:

    • PERL5OPT=-MDevel::Cover=-ignore,"t/",+ignore,"prove",-coverage,statement,branch,condition,path,subroutine prove -lrs t
    • cover
  • -after_success:

    • cover -report coveralls
  • -sudo: false

  • -notifications:

  • email: false
  • perl Makefile.PL && make && cover -test -report coveralls

What's the reason for changing all of this?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Humanstate/html-googlemaps-v3/pull/13#pullrequestreview-13977165, or mute the thread https://github.com/notifications/unsubscribe-auth/AAtHAM_4XKjMXkQAz4Fcf2bWKzT6k4KXks5rKTs8gaJpZM4LS-2i.

nigelhorne commented 7 years ago

I disagree. If you keep the 'use' above, the code is loaded even if it's never going to be used.

-Nigel

On 21/12/16 09:50, Lee J wrote:

@leejo commented on this pull request.


In lib/HTML/GoogleMaps/V3.pm https://github.com/Humanstate/html-googlemaps-v3/pull/13#pullrequestreview-13978433:

use Template;

our $VERSION = '0.09';

sub new { my ( $class,%opts ) = @_;

  • return bless( {
  • %opts,
  • points => [],
  • poly_lines => [],
  • geocoder => Geo::Coder::Google->new(
  • unless($opts{geocoder}) {
  • require Geo::Coder::Google;
  • Geo::Coder::Google->import();

just keep the use statement above, no need to do this

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Humanstate/html-googlemaps-v3/pull/13#pullrequestreview-13978433, or mute the thread https://github.com/notifications/unsubscribe-auth/AAtHAN4o1GJyFw5_cnYwiv_1ju97GDhHks5rKTy3gaJpZM4LS-2i.

nigelhorne commented 7 years ago

That's a religious war I'm not going to enter in to :-)

On 21/12/16 09:48, Lee J wrote:

@leejo commented on this pull request.


In lib/HTML/GoogleMaps/V3.pm https://github.com/Humanstate/html-googlemaps-v3/pull/13#pullrequestreview-13977865:

use Template;

our $VERSION = '0.09';

sub new { my ( $class,%opts ) = @_;

  • return bless( {
  • %opts,
  • points => [],
  • poly_lines => [],
  • geocoder => Geo::Coder::Google->new(
  • unless($opts{geocoder}) {

|if ( ! $opts{geocoder} ) { ...| as unless is a horrible construct other than in single postfix conditions :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Humanstate/html-googlemaps-v3/pull/13#pullrequestreview-13977865, or mute the thread https://github.com/notifications/unsubscribe-auth/AAtHADyYciXUg0s02KFmqKZvxS08d3_tks5rKTwngaJpZM4LS-2i.

leejo commented 7 years ago

That's the simple way I prefer to work. You can ignore it, I know you work in a different way.

The repo has a functioning .travis.yml file already so that's why i was confused by the change.

leejo commented 7 years ago

I disagree. If you keep the 'use' above, the code is loaded even if it's never going to be used.

It's going to be used by 99.99% of the use of this module.

leejo commented 7 years ago

That's a religious war I'm not going to enter in to :-)

Not really, in correct English grammar we don't use "unless" when we mean "if" and that's why my brain fails to parse it as a leading conditional. Not only that, the danger of using unless as a leading conditional is someone coming along later and introducing a second condition, which can eventually lead to a mess like:

unless ( ! $foo || $bar ) { ...

Do not use unless as a leading conditional, it stops you having to fix it later.

nigelhorne commented 7 years ago

Also, it was a good test to ensure that the geocode option was actually being used to override the default one - it wasn't until I added that code.

On 21/12/16 11:41, Nigel Horne wrote:

I disagree. If you keep the 'use' above, the code is loaded even if it's never going to be used.

-Nigel

On 21/12/16 09:50, Lee J wrote:

@leejo commented on this pull request.


In lib/HTML/GoogleMaps/V3.pm https://github.com/Humanstate/html-googlemaps-v3/pull/13#pullrequestreview-13978433:

use Template;

our $VERSION = '0.09';

sub new { my ( $class,%opts ) = @_;

  • return bless( {
  • %opts,
  • points => [],
  • poly_lines => [],
  • geocoder =>Geo::Coder::Google->new(
  • unless($opts{geocoder}) {
  • requireGeo::Coder::Google;
  • Geo::Coder::Google->import();

just keep the use statement above, no need to do this

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Humanstate/html-googlemaps-v3/pull/13#pullrequestreview-13978433, or mute the thread https://github.com/notifications/unsubscribe-auth/AAtHAN4o1GJyFw5_cnYwiv_1ju97GDhHks5rKTy3gaJpZM4LS-2i.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.2%) to 97.619% when pulling 540f560f03ffb6425528a8670974d47a85c4ea0b on nigelhorne:master into d77115a1835f1ee681319bd7598c6e51fff0787c on Humanstate:master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.2%) to 97.619% when pulling 540f560f03ffb6425528a8670974d47a85c4ea0b on nigelhorne:master into d77115a1835f1ee681319bd7598c6e51fff0787c on Humanstate:master.