Feature Request: Ability to Toggle Listener On/Off

[baronvonvaderham]

We recently ran into an issue with the listener in this package. While it's a fantastic feature, it has been problematic when attempting to run a mass update of our models, each of which triggers a recheck.

The simple solution would be to amend the end of models.py where the listeners are registered to also check for an environment variable that we could then change at will without having to interact with the code. Then we're able to easily disable the listener temporarily for things like one-off mass migrations.

I will be submitting a pull request from my fork to address this, but wanted to briefly explain the reason for it here.

[andybak]

I've actually had similar problems so it would be a useful feature.

I'd rather not hardcode the use of environment variables - I personally prefer using Django settings over env vars. If it was a Django setting there would be nothing to stop you setting this based on an environment variable of your own choosing.

@fruitschen - any thoughts on this issue in general?

[baronvonvaderham]

I think that's what I meant, the same way you handle the other variables by calling getattr(settings, 'VARIABLE'). But most often that django settings var is set via an env var on the server, so I think my brain thought of them as the same thing.

I totally agree, that allows you to do whatever logic and trickery you'd like inside your settings.py to set the value.

[fruitschen]

yes, the listeners should be optional. let us add a setting for it

[andybak]

@baronvonvaderham Happy to review/accept a PR when you're ready

[andybak]

Fixed in d6c64a6292a9d01c8d9fa5e5280efb6c9f7c22c5 - I needed it myself in a hurry

( @baronvonvaderham - I'd love a quick PR to add this to the docs if you have time.)

[claudep]

Sorry to come a bit late in the game, but I'd like to suggest a more Djangonic way of doing this. In modern Django, signal registration is suggested to happen in the AppConfig.ready method (https://docs.djangoproject.com/en/1.10/ref/applications/#django.apps.AppConfig.ready). So with this design, you can switch to a "no-signals" linkcheck by adding linkcheck.apps.BaseLinkcheckConfig into settings.INSTALLED_APPS (instead of linkcheck which is using the default LinkcheckConfig). And of course you can write your own AppConfig class to do more customization for any specific needs. Implemented in PR #63.

[baronvonvaderham]

Sorry I'm late to the party, I'm in Florida so I had the hurricane then immediately left for my company's conference in Baltimore. Looks like others have taken up the torch and took care of things while I was away.

I had originally written something almost identical to @andybak, though I was struggling to write a test case for it. But I guess that's not a critical thing to be testing.

I like @claudep's approach in that PR, and I think it works well enough with having a settings variable toggle (I'll just check my toggle variable to use either check.apps.BaseLinkcheckConfig or linkcheck depending on its value).

I could go either way, I leave it to the owners to decide, as both accomplish what I needed. Thanks!

[andybak]

Sorry for the lack of response. I'm really undecided about @claudep 's approach. I can appreciate that it's elegant in it's use of Django's existing mechanisms and I can understand the desire to reduce the proliferation of settings but I can't help but feel it makes things rather more complicated than they need to be. It's also a case of expectations.

I'd like to see more use of AppConfig 'in the wild' before I adopt it's usage in this way. I think it's the right place to put code but not sure it's the right place to look for configuration.

I'll leave this open until Claude (or others) respond. I'm prepared to have my mind changed.

[claudep]

I think I could rather easily reintroduce the DISABLE_LISTENERS setting while keeping the AppConfig refactoring. So as people preferring the settings-based configuration would not have to change anything. Would that suit you?

[andybak]

@claudep - apologies for not replying. I saw your question then completely forgot to respond.

Yes - that would be fine with me.

[claudep]

No problem, the patch has been updated.

[andybak]

Merged