search

angular / angular-hint

run-time hinting for AngularJS applications
362 stars 45 forks source link

Feat: use ng-hint="{}" as a map for which modules to include #6

Closed caguillen214 closed 10 years ago

btford commented 10 years ago

@ealtenho started on that yesterday here: https://github.com/angular/angular-hint/blob/master/load.js#L12

ealtenho commented 10 years ago

@btford, I'm not sure the direction of this issue? What are the goals here aside from what has already been done?

btford commented 10 years ago

We already started on this, but there are a few pending tasks:

ealtenho commented 10 years ago

@caguillen214 would checking

console.warn for wrong config options, like ng-hint="bar" when there is no module called bar

be similar to your work for #1 ?

ealtenho commented 10 years ago

@btford for the inclusive and exclusive modes, would this be triggered by a flag like inclusive or exclusive entered with the ng-hint tag? Or how were you envisioning this? Would inclusive or exclusive mode be the default?

btford commented 10 years ago

There are a couple of possible ways to implement this. @caguillen214 – would be good for you and @ealtenho to brainstorm and choose something.

I think it's better to avoid mixing "inclusive" and "exclusive" modes. For example, none of these make sense to me:

Other hinting systems (like jshint, jslint, eshint) allow you to scope configuration options in a way that it can make sense to have both inclusive and exclusive rules in the same configuration. Although some of Angular corresponds directly to the DOM, in angular-hint we mostly decorate the service layer.

With this in mind, we should warn when a page has more than one ng-hint. It might be good to suggest that the ng-hint occur on <html> or on the same element that has ng-app.

caguillen214 commented 10 years ago

So the implementation would be similar to what I do in hint-directive/hint-module if we decided to dynamically get the options loaded, by checking which modules have been created for the app.

It seems that it makes sense to just have like a hashmap of options we support. Since we don't have many options I feel the code would be simpler if we didn't dynamically load the ng-hint modules created and instead we just kept track of the ng-hint modules we support.

<html ng-app="app.js" ng-hint="['directive','dom']">

then something like (psuedo):

var ngHints = { 
   'all': 'all.js', 'directive' : 'hint-directive.js', 'dom': 'hint-dom.js', 'interpolation': 'hint-interpolation.js'
}
html.attr('ng-hint').value.forEach(function(hint) {
   if(ngHints[hint])
     load(ngHints[hint]);
   else
     throw new Error('No Angular-hint module found with name: '+hint);
}
caguillen214 commented 10 years ago

With this in mind, we should warn when a page has more than one ng-hint. It might be good to suggest that the ng-hint occur on or on the same element that has ng-app.

I think this is a good idea.

btford commented 10 years ago

ng-hint="['directive','dom']"

I think this is misleading in that it suggests that you could pass any angular expression in there. Because ng-hint runs before angular is bootstrapped, it's impossible to bind to an angular expression.

Let's do this:

@ealtenho can you implement (and add tests for this?)

ealtenho commented 10 years ago

@btford I've added a rough implementation of how I think this should work, but I'm having trouble getting tests to run. I assume that unit tests are the appropriate tool to use here, and I tried to start my implementation using TDD, but I can't seem to get the order of the bootstrapping correct with karma. When I run my tests I get the message that resumeBootstrap is not a function.

I have (sort of) tested by hand using the sample application and appending the various ng-hint directives.

ealtenho commented 10 years ago

I will remove my broken unit tests and start working on E2E tests for this feature.

btford commented 10 years ago

:+1:

ealtenho commented 10 years ago

This is now implemented. What remains is E2E testing as described in #7