[Phpmyadmin-devel] jshint errors
Hi all I've took some time to look at jshint checks found on our code: http://ci.phpmyadmin.net/job/phpMyAdmin-continuous/3132/violations/?#jslint I've fixed some obvious ones (like missing ;, typecast safe comparing, missing radix for parseInt), but there are still many of them. Most of them fit into following case: - ['strSave'] is better written in dot notation. This error is raised to highlight an unnecessarily verbose and potentially confusing piece of code. More detailed explanation is at: http://jslinterrors.com/a-is-better-written-in-dot-notation/ - 'type' is already defined. / 'type' used out of scope. Most of these come from following constructs: if (foo) { var bar = 1; } else { var bar = 1; } alert(bar); This is not an issue for javascript (it has no block scope for variables), it can be confusing, more details at: http://www.jshint.com/docs/#funcscope So both are more just matter of coding style rather than real bug and can be disabled in jshint. The question is whether we would prefer to hide these warnings or fix them. -- Michal Čihař | http://cihar.com | http://blog.cihar.com
On Mon, Apr 15, 2013 at 8:09 PM, Michal Čihař <michal@cihar.com> wrote:
Hi all
I've took some time to look at jshint checks found on our code:
http://ci.phpmyadmin.net/job/phpMyAdmin-continuous/3132/violations/?#jslint
I've fixed some obvious ones (like missing ;, typecast safe comparing, missing radix for parseInt), but there are still many of them. Most of them fit into following case:
- ['strSave'] is better written in dot notation.
This error is raised to highlight an unnecessarily verbose and potentially confusing piece of code. More detailed explanation is at:
http://jslinterrors.com/a-is-better-written-in-dot-notation/
- 'type' is already defined. / 'type' used out of scope.
Most of these come from following constructs:
if (foo) { var bar = 1; } else { var bar = 1; } alert(bar);
This is not an issue for javascript (it has no block scope for variables), it can be confusing, more details at:
http://www.jshint.com/docs/#funcscope
So both are more just matter of coding style rather than real bug and can be disabled in jshint. The question is whether we would prefer to hide these warnings or fix them.
I think we should fix them out. It'll increase our code quality. I'll fix some of them if no objections. Regards ! -- Chanaka Dharmarathna *Virtusa (Pvt) Ltd. | **Sri Lanka*
Hi Dne Mon, 15 Apr 2013 21:50:55 +0530 Chanaka Dharmarathna <pe.chanaka.ck@gmail.com> napsal(a):
On Mon, Apr 15, 2013 at 8:09 PM, Michal Čihař <michal@cihar.com> wrote:
Hi all
I've took some time to look at jshint checks found on our code:
http://ci.phpmyadmin.net/job/phpMyAdmin-continuous/3132/violations/?#jslint
I've fixed some obvious ones (like missing ;, typecast safe comparing, missing radix for parseInt), but there are still many of them. Most of them fit into following case:
- ['strSave'] is better written in dot notation.
This error is raised to highlight an unnecessarily verbose and potentially confusing piece of code. More detailed explanation is at:
http://jslinterrors.com/a-is-better-written-in-dot-notation/
- 'type' is already defined. / 'type' used out of scope.
Most of these come from following constructs:
if (foo) { var bar = 1; } else { var bar = 1; } alert(bar);
This is not an issue for javascript (it has no block scope for variables), it can be confusing, more details at:
http://www.jshint.com/docs/#funcscope
So both are more just matter of coding style rather than real bug and can be disabled in jshint. The question is whether we would prefer to hide these warnings or fix them.
I think we should fix them out. It'll increase our code quality. I'll fix some of them if no objections.
If we agree to do the fixes, at least first part can be mostly automated, so try to avoid manual work if it is not needed :-). -- Michal Čihař | http://cihar.com | http://phpmyadmin.net
Michal Čihař a écrit :
Hi all
I've took some time to look at jshint checks found on our code:
http://ci.phpmyadmin.net/job/phpMyAdmin-continuous/3132/violations/?#jslint
I've fixed some obvious ones (like missing ;, typecast safe comparing, missing radix for parseInt), but there are still many of them. Most of them fit into following case:
- ['strSave'] is better written in dot notation.
This error is raised to highlight an unnecessarily verbose and potentially confusing piece of code. More detailed explanation is at:
http://jslinterrors.com/a-is-better-written-in-dot-notation/
- 'type' is already defined. / 'type' used out of scope.
Most of these come from following constructs:
if (foo) { var bar = 1; } else { var bar = 1; } alert(bar);
This is not an issue for javascript (it has no block scope for variables), it can be confusing, more details at:
http://www.jshint.com/docs/#funcscope
So both are more just matter of coding style rather than real bug and can be disabled in jshint. The question is whether we would prefer to hide these warnings or fix them.
Another question: which coding style do we prefer, the one with the dot notation or with the square brackets? -- Marc Delisle http://infomarc.info
Hi, On Monday, 15 April 2013 at 10:09 PM, Marc Delisle wrote:
Michal Čihař a écrit :
Hi all
I've took some time to look at jshint checks found on our code:
http://ci.phpmyadmin.net/job/phpMyAdmin-continuous/3132/violations/?#jslint
I've fixed some obvious ones (like missing ;, typecast safe comparing, missing radix for parseInt), but there are still many of them. Most of them fit into following case:
- ['strSave'] is better written in dot notation.
This error is raised to highlight an unnecessarily verbose and potentially confusing piece of code. More detailed explanation is at:
http://jslinterrors.com/a-is-better-written-in-dot-notation/
- 'type' is already defined. / 'type' used out of scope.
Most of these come from following constructs:
if (foo) { var bar = 1; } else { var bar = 1; } alert(bar);
This is not an issue for javascript (it has no block scope for variables), it can be confusing, more details at:
http://www.jshint.com/docs/#funcscope
So both are more just matter of coding style rather than real bug and can be disabled in jshint. The question is whether we would prefer to hide these warnings or fix them.
Another question: which coding style do we prefer, the one with the dot notation or with the square brackets?
Logically, dot notation seems better for the reasons mentioned in the link explaining the error. The only problem that I see with dot notation is that we will still have to use the square bracket notation for reserved words. So that could lead to inconsistency in the convention if we have a lot of such cases where the member names are also reserved words. The same reserved words issue might cause problems if we decide to automate the fix like Michal mentioned. However, if there are not many such cases in our code base, then we can go for the dot notation and in the future, we can make sure that we don't use reserved words as member names.
-- Marc Delisle http://infomarc.info
------------------------------------------------------------------------------ Precog is a next-generation analytics platform capable of advanced analytics on semi-structured data. The platform includes APIs for building apps and a phenomenal toolset for data science. Developers can use our toolset for easy data analysis & visualization. Get a free account! http://www2.precog.com/precogplatform/slashdotnewsletter _______________________________________________ Phpmyadmin-devel mailing list Phpmyadmin-devel@lists.sourceforge.net (mailto:Phpmyadmin-devel@lists.sourceforge.net) https://lists.sourceforge.net/lists/listinfo/phpmyadmin-devel
Hi Dne Mon, 15 Apr 2013 12:39:16 -0400 Marc Delisle <marc@infomarc.info> napsal(a):
Michal Čihař a écrit :
Hi all
I've took some time to look at jshint checks found on our code:
http://ci.phpmyadmin.net/job/phpMyAdmin-continuous/3132/violations/?#jslint
I've fixed some obvious ones (like missing ;, typecast safe comparing, missing radix for parseInt), but there are still many of them. Most of them fit into following case:
- ['strSave'] is better written in dot notation.
This error is raised to highlight an unnecessarily verbose and potentially confusing piece of code. More detailed explanation is at:
http://jslinterrors.com/a-is-better-written-in-dot-notation/
So both are more just matter of coding style rather than real bug and can be disabled in jshint. The question is whether we would prefer to hide these warnings or fix them.
Another question: which coding style do we prefer, the one with the dot notation or with the square brackets?
I think PMA_messages.strSave is shorter and looks nicer than PMA_messages['strSave'], though I don't have strong opinion on that. Also with messages (the most frequent of this warning in our case) we can be pretty sure we don't use reserved words, so the dot syntax is always safe here. -- Michal Čihař | http://cihar.com | http://blog.cihar.com
Le 2013-04-16 03:06, Michal Čihař a écrit :
Hi
Dne Mon, 15 Apr 2013 12:39:16 -0400 Marc Delisle <marc@infomarc.info> napsal(a):
Michal Čihař a écrit :
Hi all
I've took some time to look at jshint checks found on our code:
http://ci.phpmyadmin.net/job/phpMyAdmin-continuous/3132/violations/?#jslint
I've fixed some obvious ones (like missing ;, typecast safe comparing, missing radix for parseInt), but there are still many of them. Most of them fit into following case:
- ['strSave'] is better written in dot notation.
This error is raised to highlight an unnecessarily verbose and potentially confusing piece of code. More detailed explanation is at:
http://jslinterrors.com/a-is-better-written-in-dot-notation/
So both are more just matter of coding style rather than real bug and can be disabled in jshint. The question is whether we would prefer to hide these warnings or fix them.
Another question: which coding style do we prefer, the one with the dot notation or with the square brackets?
I think PMA_messages.strSave is shorter and looks nicer than PMA_messages['strSave'], though I don't have strong opinion on that. Also with messages (the most frequent of this warning in our case) we can be pretty sure we don't use reserved words, so the dot syntax is always safe here.
I also think the dot notation is better, and here is another reason. When seeing the string in quotes, I always wonder whether the string 'strSave' will be shown to the user. -- Marc Delisle http://infomarc.info
Hi Dne Tue, 16 Apr 2013 06:33:52 -0400 Marc Delisle <marc@infomarc.info> napsal(a):
Le 2013-04-16 03:06, Michal Čihař a écrit :
I think PMA_messages.strSave is shorter and looks nicer than PMA_messages['strSave'], though I don't have strong opinion on that. Also with messages (the most frequent of this warning in our case) we can be pretty sure we don't use reserved words, so the dot syntax is always safe here.
I also think the dot notation is better, and here is another reason. When seeing the string in quotes, I always wonder whether the string 'strSave' will be shown to the user.
I've made pull request for this change: https://github.com/phpmyadmin/phpmyadmin/pull/257 Please review and eventually merge that. -- Michal Čihař | http://cihar.com | http://blog.cihar.com
Michal Čihař a écrit :
Hi
Dne Tue, 16 Apr 2013 06:33:52 -0400 Marc Delisle <marc@infomarc.info> napsal(a):
Le 2013-04-16 03:06, Michal Čihař a écrit :
I think PMA_messages.strSave is shorter and looks nicer than PMA_messages['strSave'], though I don't have strong opinion on that. Also with messages (the most frequent of this warning in our case) we can be pretty sure we don't use reserved words, so the dot syntax is always safe here. I also think the dot notation is better, and here is another reason. When seeing the string in quotes, I always wonder whether the string 'strSave' will be shown to the user.
I've made pull request for this change:
https://github.com/phpmyadmin/phpmyadmin/pull/257
Please review and eventually merge that.
Looks great, merged. -- Marc Delisle http://infomarc.info
HI, I also send a simple changes to fix jshint errors; 1 already defined 2 missing semicolon https://github.com/phpmyadmin/phpmyadmin/pull/260
Dne Tue, 16 Apr 2013 06:33:52 -0400 Marc Delisle <marc@infomarc.info> napsal(a):
Le 2013-04-16 03:06, Michal Čihař a écrit :
I think PMA_messages.strSave is shorter and looks nicer than PMA_messages['strSave'], though I don't have strong opinion on that. Also with messages (the most frequent of this warning in our case) we can be pretty sure we don't use reserved words, so the dot syntax is always safe here. I also think the dot notation is better, and here is another reason. When seeing the string in quotes, I always wonder whether the string 'strSave' will be shown to the user.
I've made pull request for this change:
https://github.com/phpmyadmin/phpmyadmin/pull/257
Please review and eventually merge that.
Looks great, merged.
Adam
Hi,
I've took some time to look at jshint checks found on our code:
http://ci.phpmyadmin.net/job/phpMyAdmin-continuous/3132/violations/?#jslint
I've fixed some obvious ones (like missing ;, typecast safe comparing, missing radix for parseInt), but there are still many of them. Most of them fit into following case:
- ['strSave'] is better written in dot notation.
This error is raised to highlight an unnecessarily verbose and potentially confusing piece of code. More detailed explanation is at:
http://jslinterrors.com/a-is-better-written-in-dot-notation/
- 'type' is already defined. / 'type' used out of scope.
Most of these come from following constructs:
if (foo) { var bar = 1; } else { var bar = 1; } alert(bar);
This is not an issue for javascript (it has no block scope for variables), it can be confusing, more details at:
http://www.jshint.com/docs/#funcscope
So both are more just matter of coding style rather than real bug and can be disabled in jshint. The question is whether we would prefer to hide these warnings or fix them.
About the JSHint (or JSLint), there are some opintions which can be customed by users to decide whether the warning should be evoked. Like JSHint: http://www.jshint.com/ Warn About debugging code About unsafe for..in About == null About arguments.caller and .callee About empty blocks About unsafe comparisons About assignments inside if/for/... About functions inside loops About eval About unsafe line breaks When bitwise operators are used When code is not in strict mode When variable is undefined When variable is defined but not used When blocks omit {} When new is used for side effects If we want to fix the JSHint warning, we should firstly decide which warnings should be fixed. thanks BTW, I would like to join and fix some warnings. Adam
Hi Dne Tue, 16 Apr 2013 10:51:30 +0800 adam <adamgsoc2013@gmail.com> napsal(a):
About the JSHint (or JSLint), there are some opintions which can be customed by users to decide whether the warning should be evoked. Like JSHint: http://www.jshint.com/ Warn
About debugging code About unsafe for..in About == null About arguments.caller and .callee About empty blocks About unsafe comparisons About assignments inside if/for/... About functions inside loops About eval About unsafe line breaks When bitwise operators are used When code is not in strict mode When variable is undefined When variable is defined but not used When blocks omit {} When new is used for side effects
If we want to fix the JSHint warning, we should firstly decide which warnings should be fixed. thanks BTW, I would like to join and fix some warnings.
Yes, that's why I'm asking here :-). The current set of warnings is mostly based on defaults, but we can either choose to tune it or to fix the warnings it reports. -- Michal Čihař | http://cihar.com | http://blog.cihar.com
participants (5)
-
adam -
Ayush Chaudhary -
Chanaka Dharmarathna -
Marc Delisle -
Michal Čihař