Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix only replacing wildcard at end in toNamespace #778

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Anoesj
Copy link

@Anoesj Anoesj commented Aug 16, 2020

The docs state that, when running debug.disable(), it will return the namespaces currently enabled and skipped. They are, however, often malformed. This seems to be because the toNamespace method only replaces a trailing *.

debug.enable('api:*,-*:verbose,-api:*:verbose');
const prevConfig = debug.disable();
console.log(prevConfig); // -> api:*,-.*?:verbose,-api:.*?:verbose

console.log(prevConfig); should have returned api:*,-*:verbose,-api:*:verbose.

The issue seems to be fixed by removing the $ in the replace regexp in toNamespace.

It might be nice to improve the following test too:

describe('rebuild namespaces string (disable)', () => {
	it('handle names, skips, and wildcards', () => {
		debug.enable('test,abc*,-abc');
		const namespaces = debug.disable();
		assert.deepStrictEqual(namespaces, 'test,abc*,-abc');
		// ^ could use more tests, maybe: *,test,-test,abc*,*abc,abc:*:xyz,-abc:*:xyz,-*:verbose (etcetera)
	});
	// ...
});

However, for the sake of keeping this PR clean, I haven't added that to this PR.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.692% when pulling 9f5ed03 on Anoesj:fix/wildcard-replace into 22e13fe on visionmedia:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.692% when pulling 9f5ed03 on Anoesj:fix/wildcard-replace into 22e13fe on visionmedia:master.

@@ -239,7 +239,7 @@ function setup(env) {
function toNamespace(regexp) {
return regexp.toString()
.substring(2, regexp.toString().length - 2)
.replace(/\.\*\?$/, '*');
.replace(/\.\*\?/, '*');
Copy link

@curtisman curtisman Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to set the regex to match globally, i.e. /\.\*\?/g, for the case like '*name*'

@curtisman
Copy link

I wonder how to get the maintainer's attention to get contribution merged in?

@Qix-
Copy link
Member

Qix- commented Nov 9, 2023

I'm always looking, the project is currently blocked due to Node feature parity issues when moving from CJS to ESM. Please see the roadmap pinned on the repo.

Further, .enable() and .disable() have a ton of issues as it stands and need to be rethought/reworked entirely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants