Cleanup default analytics function #113

2018-01-22T18:49:57-05:00

robinnorth commented

2018-01-22 18:49:57 -05:00

(Migrated from github.com)

A very small cleanup of the default analytics functionality -- moved a check that did nothing to somewhere more useful and deleted a comment that didn't reflect what the function actually does.

BehindTheMath commented

2018-01-22 20:16:52 -05:00

(Migrated from github.com)

moved a check that did nothing to somewhere more useful

I don't understand how this is better than the original. The check was in case a user set options.analytics to a truthy value that wasn't a function, in which case it was replaced by a dummy function, so it wouldn't throw an error when we try calling it. Why is it better to have the check later on?

>moved a check that did nothing to somewhere more useful I don't understand how this is better than the original. The check was in case a user set `options.analytics` to a truthy value that wasn't a function, in which case it was replaced by a dummy function, so it wouldn't throw an error when we try calling it. Why is it better to have the check later on?

robinnorth commented

2018-01-23 04:20:41 -05:00

(Migrated from github.com)

IMO it's more useful to have the check right before we try and call the function to make certain that it's not going to throw an error -- when dealing with variables that could be set by a user, I always like to code defensively.

It would be even more useful though if the options check also assigned the default analytics function if the user supplied a truthy value that wasn't a function, so that all bases are covered. I will make it so.

IMO it's more useful to have the check right before we try and call the function to make certain that it's not going to throw an error -- when dealing with variables that could be set by a user, I always like to code defensively. It would be even more useful though if the options check also assigned the default analytics function if the user supplied a truthy value that wasn't a function, so that all bases are covered. I will make it so.

BehindTheMath commented

2018-01-23 10:30:09 -05:00

(Migrated from github.com)

What if I want to disable Pjax from handling analytics? I could set options.analytics to a void function, but I think it would be better to set it to false in that situation, and add a truthy check first, instead of just checking for a function. Then, if the user sets it to a truthy value that's not a function, you can just set it to false or null.

What if I want to disable Pjax from handling analytics? I could set `options.analytics` to a void function, but I think it would be better to set it to `false` in that situation, and add a truthy check first, instead of just checking for a function. Then, if the user sets it to a truthy value that's not a function, you can just set it to `false` or `null`.

robinnorth commented

2018-01-23 10:39:38 -05:00

(Migrated from github.com)

That's a good point, I will implement as you have described.

BehindTheMath commented

2018-01-24 17:55:02 -05:00

(Migrated from github.com)

@robinnorth Can you resolve the merge conflicts?

robinnorth commented

2018-01-24 18:21:40 -05:00

(Migrated from github.com)

Certainly, will rebase and resolve once #114 is merged

BehindTheMath commented

2018-01-24 18:31:36 -05:00

(Migrated from github.com)

I think the conflict is with the current master/HEAD, because of e7935d9c74. So you can resolve it without waiting for #114, as long as you rebase on the current master and incorporate the changes from that commit.

I think the conflict is with the current master/HEAD, because of e7935d9c7412e75adf07e9b41471e937bd17eb43. So you can resolve it without waiting for #114, as long as you rebase on the current master and incorporate the changes from that commit.

BehindTheMath commented

2018-01-24 18:58:47 -05:00

(Migrated from github.com)

LGTM. Is this ready to be merged?

🎉 1

BehindTheMath (Migrated from github.com) approved these changes 2018-01-24 18:59:12 -05:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: iLoveElysia/pjax#113