If var is a nodejs require, don't check the name

Fixes #31
var fs = require("fs")
In the above case, it's fine that variable is named fs because
this is a pattern in nodejs (and other common module implementations)
The parser will check that A/ it's a require function call, and
B/ that the name of the module corresponds to the name of the var

parsers/fileparser.py

@@ -1,6 +1,6 @@
 import re
 
-from jsparser import JSFileParser
+from jsparser import JSFileVisitorHandler
 from lineparser import LineParser, FileLines
 from functionparser import FunctionParser
 from variableparser import VariableParser
@@ -35,21 +35,21 @@ def get_file_data_from_content(src_file_name, src_file_content):
 	"""Use this to gather data for file, given its content.
 	Will raise a jsparser.ParsingError if the syntax is incorrect"""
 
-	parser = JSFileParser(src_file_content)
+	visitor_handler = JSFileVisitorHandler(src_file_content)
 
 	line_parser = LineParser()
-	parser.add_visitor(line_parser)
+	visitor_handler.add_visitor(line_parser)
 
 	function_parser = FunctionParser()
-	parser.add_visitor(function_parser)
+	visitor_handler.add_visitor(function_parser)
 
 	variable_parser = VariableParser()
-	parser.add_visitor(variable_parser)
+	visitor_handler.add_visitor(variable_parser)
 
 	class_property_parser = ClassPropertyParser()
-	parser.add_visitor(class_property_parser)
+	visitor_handler.add_visitor(class_property_parser)
 
-	parser.parse()
+	visitor_handler.visit()
 
 	src_file_functions = function_parser.functions
 	src_file_variables = variable_parser.variables

parsers/functionparser.py

@@ -43,7 +43,7 @@ def __repr__(self):
 		return "Function " + self.name + ", line " + str(self.line_number) + " (" + str(self.signature) + ") (" + str(len(self.lines.all_lines)) + " lines of code)"
 
 
-class FunctionParser:
+class FunctionParser(object):
 	"""
 	Parser/visitor for functions
 	"""
@@ -59,14 +59,14 @@ def add_function(self, name, body, line_number, signature, start_pos, end_pos, s
 		function = FunctionData(name, inner_body, line_number, signature, inner_body_start_pos, inner_body_end_pos, [], False)
 		self.functions.append(function)
 
-	def add_var(self, function, name, line_number, start, end):
+	def add_var(self, function, name, is_nodejs_require, line_number, start, end):
 		is_already_there = False
 		for var in function.variables:
 			if var.name == name and var.line_number == line_number:
 				is_already_there = True
 
 		if not is_already_there:
-			function.variables.append(VariableData(name, line_number, start, end))
+			function.variables.append(VariableData(name, is_nodejs_require, line_number, start, end))
 
 	def get_last_function(self):
 		if len(self.functions) > 0:
@@ -89,11 +89,10 @@ def get_functions_nesting(self, position):
 
 	def visit_VAR(self, node, source):
 		if self.is_in_function(node.start):
-			for subvar_node in node:
-				if getattr(subvar_node, "initializer", False):
-					self.add_var(self.get_last_function(), subvar_node.value, subvar_node.lineno, subvar_node.start, subvar_node.initializer.end)
-				else:
-					self.add_var(self.get_last_function(), subvar_node.value, subvar_node.lineno, subvar_node.start, subvar_node.end)
+			variable_parser = VariableParser()
+			variables = variable_parser.extract_variables(node)
+			for variable in variables:
+				self.add_var(self.get_last_function(), variable["name"], variable["is_nodejs_require"], variable["line_number"], variable["start_pos"], variable["end_pos"])
 
 	def visit_RETURN(self, node, source):
 		functions = self.get_functions_nesting(node.start)

parsers/jsparser.py

@@ -13,15 +13,30 @@ def get_msg(self, error):
 	def get_line_number(self, error):
 		return int(str(error).split("\n")[1].split(":")[1])
 
-class JSFileParser:
-	"""The js file parser can parse js source code and iterate over its AST.
+class JSFileParser(object):
+	def __init__(self):
+		self.parsing_error = None
+
+	def is_parsed(self):
+		return self.parsing_error == None
+
+	def parse(self, source):
+		ast = None
+		try:
+			ast = jsparser.parse(source)
+		except jsparser.ParseError as error:
+			self.parsing_error = ParsingError(error)
+		return ast
+
+class JSFileVisitorHandler(object):
+	"""The js file visitor can visit a parsed js source code and iterate over its AST.
 	It can accept any number of visitors (via add_visitor).
 	
 	Example:
 
-	parser = JSFileParser(content)
-	parser.add_visitor(MyVisitor())
-	parser.parse()"""
+	visitor_handler = JSFileVisitorHandler(content)
+	visitor_handler.add_visitor(MyVisitor())
+	visitor_handler.visit()"""
 
 	CHILD_ATTRS = ['value', 'thenPart', 'elsePart', 'expression', 'body','exception', 'initializer',
 	'tryBlock', 'condition','update', 'iterator', 'object', 'setup', 'discriminant', 'finallyBlock',
@@ -31,26 +46,13 @@ def __init__(self, source):
 		self.visited = list()
 		self.source = source
 		self.visitors = []
-		try:
-			self.root = jsparser.parse(self.source)
-		except jsparser.ParseError as error:
-			raise ParsingError(error)
-
-	def review(self, file_data, message_bag):
-		try:
-			ast = jsparser.parse(file_data.content)
-		except jsparser.ParseError as error:
-			message_bag.add_error(self, self.extract_error_msg(error), self.extract_error_line(error))
 
+		self.parser = JSFileParser()
+		self.root = self.parser.parse(self.source)
+
 	def get_visitors(self):
 		return self.visitors
 
-	def look4Childen(self, node):
-		for attr in self.CHILD_ATTRS:
-			child = getattr(node, attr, None)
-			if isinstance(child, jsparser.Node):
-				self.visit(child)
-
 	def add_visitor(self, visitor):
 		self.visitors.append(visitor)
 
@@ -75,12 +77,21 @@ def exec_visitors_function(self, function_name, source, node=None):
 			elif visitor_func and not node:
 				visitor_func(source)
 
-	def parse(self):
-		self.exec_preprocess_visitors(self.source)
-		self.visit()
-		self.exec_postprocess_visitors(self.source)
+	def visit(self):
+		if self.parser.is_parsed():
+			self.exec_preprocess_visitors(self.source)
+			self._walk_node()
+			self.exec_postprocess_visitors(self.source)
+		else:
+			raise self.parser.error
+
+	def _look_for_childen(self, node):
+		for attr in self.CHILD_ATTRS:
+			child = getattr(node, attr, None)
+			if isinstance(child, jsparser.Node):
+				self._walk_node(child)
 
-	def visit(self, root=None):
+	def _walk_node(self, root=None):
 		if not root:
 			root = self.root
 
@@ -91,9 +102,9 @@ def visit(self, root=None):
 
 		self.exec_visitors_on_node(root, self.source)
 
-		self.look4Childen(root)
+		self._look_for_childen(root)
 		for node in root:
-			self.visit(node)
+			self._walk_node(node)
 
 
 if __name__ == "__main__":
@@ -171,7 +182,7 @@ def visit(self, root=None):
 			}
 		}
 	}"""
-	parser = JSFileParser(content)
+	parser = JSFileVisitorHandler(content)
 
 	from functionparser import FunctionParser
 	function_visitor = FunctionParser()
@@ -194,12 +205,19 @@ def visit(self, root=None):
 		return object.doSomething();
 	}
 	"""
-	parser = JSFileParser(function_return_code)
+	parser = JSFileVisitorHandler(function_return_code)
 	parser.parse()
 
 	var_usage = """var a = multiply/2;
 	var b = a*multiply+(multiply/multiply)
 	"""
-	JSFileParser(var_usage)
+	JSFileVisitorHandler(var_usage)
+
+	bare_return = "return object.doSomething();"
+	try:
+		JSFileVisitorHandler(bare_return)
+		assert False, "Should have failed"
+	except jsparser.ParseError as error:
+		assert True
 
 	print "ALL TESTS OK"

parsers/variableparser.py

@@ -6,35 +6,72 @@ class VariableData(visitor.Entity):
 	"""Passed to each reviewer, as an element of the variables list, inside file_data.
 	Instances of this class have the following attributes:
 	- name: the name of the variable
+	- is_nodejs_require: was the variable initialized as a nodejs required dependency
 	- line_number: the line number in the file where the variable occurs"""
 
-	def __init__(self, name, line_number, start_pos, end_pos):
+	def __init__(self, name, is_nodejs_require, line_number, start_pos, end_pos):
 		super(VariableData, self).__init__(line_number, start_pos, end_pos)
 		self.name = name
+		self.is_nodejs_require = is_nodejs_require
 
 	def __repr__(self):
-		return "Variable " + self.name + "(line " + str(self.line_number) + ")"
+		return "Variable " + self.name + " (line " + str(self.line_number) + ")"
 
 class VariableParser:
 	def __init__(self):
 		self.variables = []
 
-	def add_var(self, name, line_number, start, end):
+	def add_var(self, name, is_nodejs_require, line_number, start, end):
 		is_already_there = False
 		for var in self.variables:
 			if var.name == name and var.line_number == line_number:
 				is_already_there = True
 
 		if not is_already_there:
-			self.variables.append(VariableData(name, line_number, start, end))
+			self.variables.append(VariableData(name, is_nodejs_require, line_number, start, end))
 
-	def visit_VAR(self, node, source):
+	def is_initializer_call_to_require(self, initializer):
+		return initializer.type == "CALL" and initializer[0].value == "require"
+
+	def is_var_being_initialized(self, node):
+		return getattr(node, "initializer", False)
+
+	def is_string_value(self, node, value):
+		return node.type == "STRING" and node.value == value
+
+	def is_nodejs_require_var_init(self, initializer, var_name):
+		is_nodejs_require = False
+		if self.is_initializer_call_to_require(initializer):
+			if self.is_string_value(initializer[1][0], var_name):
+				is_nodejs_require = True
+		return is_nodejs_require
+
+	def extract_variables(self, node):
+		variables = []
 		for subvar_node in node:
-			if getattr(subvar_node, "initializer", False):
-				self.add_var(subvar_node.value, subvar_node.lineno, subvar_node.start, subvar_node.initializer.end)
+			if self.is_var_being_initialized(subvar_node):
+				is_nodejs_require = self.is_nodejs_require_var_init(subvar_node.initializer, subvar_node.value)
+				variables.append({
+					"name": subvar_node.value,
+					"is_nodejs_require": is_nodejs_require,
+					"line_number": subvar_node.lineno,
+					"start_pos": subvar_node.start,
+					"end_pos": subvar_node.initializer.end
+				})
 			else:
-				self.add_var(subvar_node.value, subvar_node.lineno, subvar_node.start, subvar_node.end)
+				variables.append({
+					"name": subvar_node.value,
+					"is_nodejs_require": False,
+					"line_number": subvar_node.lineno,
+					"start_pos": subvar_node.start,
+					"end_pos": subvar_node.end
+				})
+		return variables
 
+	def visit_VAR(self, node, source):
+		variables = self.extract_variables(node)
+		for variable in variables:
+			self.add_var(variable["name"], variable["is_nodejs_require"], variable["line_number"], variable["start_pos"], variable["end_pos"])
 
 if __name__ == "__main__":
 	print "NO TESTS TO RUN"

reviewers/naming.py

@@ -42,6 +42,11 @@ def review_all_names(self, vars, functions, message_bag):
 		all_objects = vars + functions
 
 		for object in all_objects:
+			# Name might be a nodejs var assignment, in which case it's fine
+			if getattr(object, "is_nodejs_require", False):
+				if object.is_nodejs_require:
+					continue
+
 			words = extractwords.get_all_words_from_line(object.name)
 			if getattr(object, "signature", None):
 				for arg in object.signature:

run_tests.py

@@ -42,11 +42,11 @@ def run_integration_tests():
 
 			# Gather data about the file to be reviewed
 			file_data = None
-			try:
-				file_data = fileparser.get_file_data_from_file(script_dir + os.sep + item)
-			except Exception as error:
-				print error
-				break;
+			#try:
+			file_data = fileparser.get_file_data_from_file(script_dir + os.sep + item)
+			#except Exception as error:
+			#	print error
+			#	break;
 
 			# Review the file
 			result = reviewer.review(file_data)

testscripts/expected.py

@@ -1,4 +1,5 @@
 results = {
+	"shortnodename.js": {},
 	"oneline.js": {
 		"messages": [
 			"1 Name b doesn't mean anything",

testscripts/shortnodename.js

@@ -0,0 +1 @@
+var fs = require("fs");