mariano / node-db-mysql

MySQL database bindings for Node.js
http://nodejsdb.org
150 stars 30 forks source link

reserved word as method name: delete #70

Open Satyam opened 12 years ago

Satyam commented 12 years ago

delete is a reserved word in JavaScript and should not be used as an identifier. A future interpreter might be more strict that current ones and might fail on this code. Many tools also fail, just look at the sample code shown and you'll see that the record deletion example has the keyword delete in the color of reserved words, not in that of methods. IDEs, code validators and other tools might failr or issue pointless warnings

Please deprecate delete in favor of some other name such as erase or del. You might keep delete as an alias for a while, but do not promote its use. Eventually, some JavaScript interpreter will complain about it and you'll have to change it in a rush.

arcanis commented 12 years ago

"+1"

qraynaud commented 12 years ago

Still, delete is also already specified in ECMAScript. It's not a reserved keyword for future use. It's unlikely that V8 will change its implementation details around the delete keyword. It's rather not urgent though I agree that something should be done in the future about this. Sad because delete is really appropriate there.

For those that are not used to JS and reading this, delete is removing an attribute from an object. Eg:

var obj = { test: 42 };
('test' in obj) // true
delete obj.test;
('test' in obj) // false
qraynaud commented 12 years ago

Ok. I was wrong in my previous comment. I double checked the ECMAScript 5 specifications and reserved keywords can't be used à identifiers. But they can be used as object properties. So delete is perfectly okay after a dot as every other JS keyword. Those are valid :

var obj = {};
obj.if = 4;
obj.delete = 8;
obj.throw = 'toto';

This bug is invalid and can be closed.

Satyam commented 12 years ago

Everything can be used as an object property by using this form:

obj['1+1=2'] = 4

While on the other hand, if you had a delete property, this would not work:

with (obj) { delete = 8; }

That it works doesn't mean it is a recommended practice. I still think that an alternate synonym should be used (such as 'erase') and delete deprecated.

The fact that the language is tolerant doesn't mean it has to be abused.

Syntax highlighters and syntax checkers would get confused and warn you about this. Many companies have internal software standards where a code needs to be send through syntax checkers before promoting it to production, and your 'delete' method would make their code fail such a test.

qraynaud commented 12 years ago

No, it's not that the language is tolerant. It's that ECMAScript 5 has been designed to allow such use of keywords because they are right.

The "with issue" your are pointing is a non issue. Strict mode is showing the direction there. The with keyword should be avoided at all costs. Strict mode forbids it and it is likely it will be completely suppressed in the future.

My point is it is allowed in ES5 and that there is nothing official that says it should be avoided on modern implementations as V8. Since this is a V8 only module, I don't see the issue there.

Satyam commented 12 years ago

Look at the example page, the highlighter used there gets confused and it is colouring delete as a keyword, paste that example into pastebin, jsfiddle or any IDE such as Eclipse and NetBeans and when they all fail, do go to the authors of each and every one of those with the reference to the ECMAScript section that supports your position and ask them to change their applications to suit your fancy. Hire lawyers to defend your position, start an ONG to support it, challenge everyone to a duel. Still, none of that will change the fact that delete remains a bad choice.

qraynaud commented 12 years ago

I don't think that some color on properties with the same names as keywords is a problem. I'm pretty happy with some color on delete. I perfectly understand that people that develops color highlighters don't want to write a full lexer/parser for achieving the purpose and thus use simpler regexp to do that.

Knowing that, any normal developer can read the code and won't cry over this.

In the same spirit C++ devs are pretty used to have incorrect if not erroneous auto completion if not even coloration on their code when they use advanced templating. The world is not perfect.

This does not mean there is a reason good enough to make this change here. Since the API is not infriging the standard and since delete is the obvious choice (I say obvious because it's the function you want to try without even reading the API documentation), it seems to go a little overboard to go as far as deprecating delete. But if you want, you can perfectly alias delete to prevent its use and prevent bad coloration on your side :

mysql.Query.prototype.del = function() { this['delete'].apply(this, arguments); };
patsimon commented 11 years ago

I agree that NOT using the "delete" on the method will make this product developer friendly and ease the confusion. It may be ok "to the spec" but dev's won't be reading the spec - they will simply be using this library.

qraynaud commented 11 years ago

You really think that? The first time I tried this library, I completely forgot that delete was a keyword because I had the MySQL verb in head and used it. It worked and I never needed to investigate further.