-
-
Notifications
You must be signed in to change notification settings - Fork 21
lint, update namespace/debug to be plugins, and their logic #12
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
Conversation
@@ -1,10 +1,7 @@ | |||
'use strict'; | |||
|
|||
require('mocha'); | |||
require('should'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't all have mocha
globally, that's a bad practice. users shouldn't be forced to install it with -g
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will also work if it is installed locally when just npm install && npm test
, without requiring it.
I don't know how much it is a bad practice, but hate to waste time to install shitty-mitty-bitty-biggy modules every time i'm going to test or pr to some project.
"every time i'm going to test" meaning, that there are many project that just have one devDependency and it is some testing framework.
But yea, preferences, religious things.. anyway. :)
edit: Another question is that that mocha is bad in general, exactly because this globals. I'm agree here - that is bad practice. That's why there is absolutely better and smaller testit
, which is almost drop-in replacement for mocha
. That's why i created assertit
on top of testit
with few lines enhancement and using it everywhere from one year or so.
as I mentioned in a comment, I think some of this belongs in Before we do that though, what are some other options for initializing custom namespaces like the ones in There are lots of different ways we can do this. We were planning on removing most of the debug code from Once we got the debug stuff working in a way that seems easy to maintain and works the way we wanted it to, we were going to implement basic, "auto-namespaced" thougths? |
just saw that line, sorry! yeah that would be great. I can create |
Haha. Yea, yea, no problem, i'm not greedy.
Mm, yes? If you talking for the two plugins in utils, yes. I intentionally set them here and PR here - for review and to start from somewhere - it was the easier way, get it work here, get some thoughts from you, and get them out to separate plugins and use them here again.
It's done, buddy :D All the user need is to use the edit: okey, want PR to |
Btw, also the cool thing and idea behind this second namespace propery |
okay, I was wondering... to clarify then:
|
Yes, it works exactly in that way. Mm, let me look the tests again - i'm almost sure. |
Yes, asbolutely! that's two of the it('should take a namespace', function() {
var debug = app.debug('bar');
debug('debugging %s', 'whatever');
assert.equal(app._namespace, 'base');
assert.equal(app._debugNamespace, 'base:bar');
});
it('should take a complex namespace', function() {
app.debug('bar:one', 123, 'foo:qux:two', 'four', 'five:six')('hello world');
//=> base:bar:one:foo:qux:two:four:five:six hello world
assert.equal(app._namespace, 'base');
assert.equal(app._debugNamespace, 'base:bar:one:foo:qux:two:four:five:six');
}); where, if you are in assemble, edit: so commented |
while (i < len) { | ||
var ns = namespaces[i++]; | ||
app.debug[ns] = debugFactory.call(app, ns); | ||
app.debug[ns].color = app.debug.color; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this line is working the way you think. Since app.debug
is acting as a debugFactory
instead of a debugger method, it doesn't have a .color
on it.
If you're going the approach that a user needs to do app.debug()('this is a message')
to access the default debugger, then I think that debugger needs to be cached with the namespace and the color from that can be used on the children to match the application color.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this line is working the way you think.
Haha, actually yea, i noticed it.
If you're going the approach that a user needs to do
It's intentional, to be used as compose thingy for other namespaces (methods on app.debug.*
)
God! New commented approach works as expected. Changed in no minute, that's awesome. Okey, i can start with PR to |
…ebugNamespace and _debugSuffix
sounds good thx |
K I'm closing this pr since we discussed other options with base-debug and base-namespace. I also want to simplify the |
Yea, I'm trying to come to base-debug and base-namespace. I hope it will be this week. I have them locally. |
Hey no worries, take your time! |
should
removed - not used anywhere.mocha
require removed - blocks us, we all have it globally.gulp
, maybe would revert it, but it's a good thing anywayapp._namespace
is more stable nowapp._debugNamespace
, because we can't just overwrite_namespace
from debug plugin, or we can it would be unnecessary bonus job.Constructor.debug
thingy and then calling it from app's constructur withdebug(this)
debug
method accept string arguments, and also support splitting by:
this.debug.helper
,this.debug.view
, etcapp.debug.my = app.debug('one')
namespace
should be separate plugin_debugNamespace
and_namespace
example.js
I can PR to base-debug, templates, assemble, generate and verb to get the change (it would be just removeing the
debug(this)
thing from their constructors). And we should createbase-namespace
.