Opened 13 years ago

Closed 8 years ago

#2571 closed enhancement (wontfix)

[patch] [ccla] syntax checking feature

Reported by: csawyer@… Owned by: James Burke
Priority: low Milestone: future
Component: Core Version: 0.9
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by James Burke)

Alter dojo.require() so that when code is pulled in from sources, we verify it against a syntax checker.. the beauty is then, your code STAYS clean because everytime you run it, it will get mad if you write bad syntax like I do. This is only true if you load dojo.syntax.*. Uses jslint.com's syntax checker currently. Other checkers could be used, but no others are supported currently.

it will only start checking after you have dojo.syntax.* loaded.

features:

allow stopping on error, or just warning on error. djConfig.syntaxWarn? Follow dojo JS guidelines as much as possible. allow it to skip dojo srcs, and only get upset with stuff in your custom namespace.

current implementation at: http://www.yuma.ca/tech/js/dojo/src/syntax/

demo/testcase: http://www.yuma.ca/tech/js/testsyntax.html

Attachments (2)

syntax.diff (100.4 KB) - added by csawyer@… 13 years ago.
patch of demo implementation.
syntaxDiff.txt (102.1 KB) - added by guest 12 years ago.
2007-4-12 revision of syntax checker.

Download all attachments as: .zip

Change History (17)

Changed 13 years ago by csawyer@…

Attachment: syntax.diff added

patch of demo implementation.

comment:1 Changed 12 years ago by James Burke

csawyer: sorry for not taking a look sooner at this. I've been busy with 0.9 changes and work. I won't be dealing with this patch soon, but some comments:

You don't need loader enhancements to plug this in. You can override dojo.hostenv.getText(), and look at the file extension. If it ends in .js, run it through the syntax checker before returning the value. Something like:

{{{{ dojo.syntax._realGetText = dojo.hostenv.getText; dojo.syntax.getText = function(uri, async_cb, fail_ok){

var contents = dojo.syntax._realGetText.apply(this.hostenv, arguments); if(contents && /*do checking for if syntax is loaded and is a .js file*/){

Do syntax checking in here.

} return contents;

} dojo.hostenv.getText = dojo.syntax.getText; }}}

Since dojo.syntax is always included after the real getText() is defined, this should work out, and allow the module to stand on its own.

comment:2 Changed 12 years ago by James Burke

Ack. I messed up the formatting. Hopefully you get the idea.

comment:3 Changed 12 years ago by guest

Find a new attachment that brings syntax checking hopefully up to par. I got the idea jburke, thank you!

This changes a lot in the checker as well, plus overrides getText.

Changed 12 years ago by guest

Attachment: syntaxDiff.txt added

2007-4-12 revision of syntax checker.

comment:4 Changed 12 years ago by Adam Peller

Milestone: 0.9beta0.9

we can't hold up beta for this. Moving to 0.9

comment:5 Changed 12 years ago by Adam Peller

This needs to go in dojox, due to external packages and varying licenses. Seems like the right place for it anyhow. I suppose the issue then is just getting the right hook for dojo.require()? It would be neat if we could use an aspect-oriented approach rather than hacking up dojo.require. Can we use after advice and resurrect the path from the return value of require()?

I suggest we resolve that issue then get an updated patch for dojox. Probably shouldn't be a 0.9 deliverable at this point.

comment:6 Changed 12 years ago by James Burke

peller: I agree. I've been talking with Tom about the right place for this in dojox. I was keeping it as a 0.9 issue for now, because I feel bad about letting a patch sit this long. But I agree, if I run out of time (which seems likely), I can push to 1.0.

comment:7 Changed 12 years ago by James Burke

Milestone: 0.91.0

I wanted to get this in for 0.9, but I'm not going to make it. Sorry to hold this off for so long.

comment:8 Changed 12 years ago by Adam Peller

Cathy, sorry to ask you this after us all waiting so long, but would you be interested in helping us get this updated to 1.0 APIs and experimenting with solutions for the dojo.require hooks/AOP?

comment:9 Changed 12 years ago by Adam Peller

suggest we push to 2.0 or punt

comment:10 Changed 12 years ago by Adam Peller

Milestone: 1.02.0

comment:11 Changed 12 years ago by alex

Milestone: 2.01.3

Milestone 2.0 deleted

comment:12 Changed 11 years ago by James Burke

Description: modified (diff)
Milestone: 1.3future

comment:13 Changed 8 years ago by ben hockey

Keywords: needsreview added
Priority: highlow

is this still relevant?

comment:14 Changed 8 years ago by bill

Not sure what "relevant" means in this case but I'd vote to just close as wontfix. I don't think we want to include all this "dojo.syntax" code into core, both for licensing and other reasons. If there is still interest in this maybe it would be a good project to add to github and register in packages.dojotoolkit.org?

comment:15 Changed 8 years ago by Adam Peller

Keywords: needsreview removed
Resolution: wontfix
Status: newclosed

Agreed.

Note: See TracTickets for help on using tickets.