Repository navigation
debug.enable() flushes enabled namespaces #425
Description
Activity
The new implementation allows full control (i.e. override of the original
DEBUGsettings), which seems desireable (even more in the context of changing this dynamically later on, see issue #433 ).
I submit that the main problem here is that public methodenable()is not documented.
Actuallydebuglacks an API reference document – does obviously not prevent it from being appreciated and widely used, but makes integration with other tools more difficult...Reacted by Greg Connell- added a commit that references this issue
on Nov 8, 2017 I disagree with @geonanorch slightly - I feel like
.enable()should be able to take-*and.disable()should be able to take*in order to clear the enabled namespaces.In fact, the default behavior if no namespaces are passed (e.g. literally
debug.enable()anddebug.disable()alike) should be to implicitly pass*, which would enable/disable all namespaces, respectively.@Qix- I don't think that we disagree, your proposal for a reset makes plenty of sense!
Personally I prefer the 'no parameter provided' approach to a wildcard, i.e..enable()rather than.enable(*), but that is cosmetic really.Well realistically both should be supported, I think.
It seems this issue is a wont-fix? @Qix- @geonanorch ?
In other words: the bug is a feature :)
The implicit and explicit
*and-*seems to be a different feature/issue.Yeah, same thing really. The fact that
.enable()overwrites everything is what trips people up, methinks. Hence why I saiddebug.disable('*'); debug.enable(...)should be how users would migrate, and that calls to.enable()and.disable()should instead modify the existing namespaces.In fact, I'd almost say that we get rid of
.disable()as you merely need to negate any patterns passed to.enable()to get the exact same functionality in that case..disable()is nice for convenience and code readability though.4 remaining items
Mmmm... I feel in part responsible for the present situation, I'm afraid that I had not seen the full picture when I raised #433. Looking back at all the comments and related issues, my understanding is that:
- by default we want
enable()to update the configuration, not replace it. There seem to be a large agreement on that point, so there is also a strong need for a fix - we also want to have full control and be able to set "debug exactly 'this'", if only for convenience (only look at one component at a time) or for automation purpose (expect a specific output)
- finally there is a discussion on whether do keep
disable()or use a negating pattern inenable(). Personally I agree with @mblarsen that keepingdisable()is more readable, but (no offense @Qix- ) I have no qualms about providing 2 paths to the same outcome: internally they would be linked. Maybe aset()method is what we need to preempt that discussion ;-)
If we can agree on the above, then we could implement the following with minimal pain:
enable(id)anddisable(id)only update state forid, nothing elseenable(=id)anddisable(=id)set absolute state --with '*' as wildcard id to reset everything- optionally
set()can support all cases:set(id),set(-id),set(=id)(I am not pushing forset(), feels a bit like overkill, but cheap to add)
In all cases, clear documentation :-D
This issue has the label "pr-welcome", I am willing to do that if
a) there is an agreement (with or withoutset()?)
b) nobody else is candidate (I have little time to offer)- by default we want
I agree on the above.
Any function signature would be ok for me...Note:
- disable(=id): I don't understand what it does?
- For my personal use, "set" and would not be necessary
Thinking futher (if you want to):
- Could also be: disableAll(), enable(id), disable(id)... ==> enable(=id) is for me disableAll() + enable(id). Would that not be more clear?
- This is a breaking change (and it should be nice to avoid those). So adding "disable(id)" and "enableAlso(id)" would not break anything... while enabling the expected behavior.
My two cents... if you want them.
@jehon thank you for providing feedback. I agree with you that
disable(=id)is not clear, in fact it's horrible ;-)
With '=' I wanted to stay in line with Qix- initial comment and avoid adding new methods: adding the---All()variants brings us to 4 methods, which is maybe not ideal either for clarity... But there are other ways. How about this revised proposal:enable(id)/disable(id)change only 'id'enable(*)/disable(*)enable / disable allenable()/disable()is obsolete but supported for backward-compatibility (if required?)
@Qix-, this is basically your old proposal, what do you think? If you want to make a call, someone can then start working on a PR (volunteers?...)
Having trouble with this today:
const a = DEBUG('alpha'); const b = DEBUG('beta'); DEBUG.enable('alpha'); DEBUG.enable('beta'); a('hi'); b('hi');
beta hi +0msI'd really love a way to toggle on and off individual
id's. We don't need to make a breaking change here I'd settle forenableIdanddisableIdor any new method set.Or even if I could access
DEBUG.namespaceand possibly doDEBUG.enable(`${DEBUG.namespace},alpha`)sort of hope$PATHworks in bash?@reggi yes the case is clear ;-)
In my previous comment I tried so summarize the discussion into a proposal, waiting for @Qix- to let us know how it should go.@geonanorch
Water did pass under the bridge... I think we can go with the proposal :-)@geonanorch
Don't be afraid... I don't have access neither, but just clone the repo, work on your cloned repository, and then make a PR. That's easy. I have already done it on other open source project.
This may help: https://docs.github.com/en/free-pro-team@latest/github/collaborating-with-issues-and-pull-requests/creating-a-pull-request
When you make the pull request, you choose the visionmedia/debug - master as the target (aka this repo).I'm quite certain they meant they wanted to confirm the work that needs to be done, not that they don't know how to make a PR.
I've been neglecting this repo for a while now due to some pandemic-related stresses. I apologize.
I'm quite hesitant on changing anything at the present moment because of what the v5 release might look like. If we are to change the programmatic nature of things, I think we'll keep
.enableand.disableas-is but deprecated in a minor release. Then, we'll introduce.configure()(better name suggestions are appreciated) to modify the enabled namespaces in a more intuitive way as I've described before.I need to think a bit more about the best way to go about this given that v5 is going to be more or less a large rewrite. I also need to discuss some ownership matters with the other maintainers before I can say anything definitively.
Apologies for the vagueness. Things go slow with such a large and sensitive project.
@jehon as Qix surmised I raised PRs before ;-) Thanks for the intention, though (you might want to PM in such cases, which are not directly relevant to an issue)
@Qix- no problem at all. Github is full of repos where the owner no longer answers, so I for one am glad you got back to us. And understood, let's wait and see what v5 brings!
Should this issue be closed or requalified, then?I'll close this when it's resolved in the new version. :)
Hi All, @geonanorch @Qix-
In version 4.3.1 of debug I have been trying to adopt the features of enable() and disable(). If I were to use the examples and add in additional printouts I can see it doesn't work.
let debug = require('debug');
debug.enable('foo:*,-foo:bar');
console.log('debug.enabled("foo")=', debug.enabled('foo'));
let namespaces = debug.disable();
debug.enable(namespaces);
debug('DO STUFF');It seems like this bug listed here is at the source of this issue. Is this the problem you are talking about? And if yes, when would this build be released because it seems like it should come out soon?
Coming back to this issue (and the one I had filed), is there now a way to do this?
Pre-3., debug.enable() did not override the previously set namespaces. This allowed to have debug.enable(namespace) in each file and enable/disable as needed on a per-file basis.
How can this behavior be reproduced with 3. versions?Gentle ping. I would still like this functionality to be restored to control logging file by file, instead of globally.
Hey there! This looks like something I could help with. I'd be happy to take a look and see what we can do.
(Found via Echeo)
Calling
enableis disruptive to any enabled namespaces, which has changed in the recent minor version, caused by #409