-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix some basic JS CS #9328
Fix some basic JS CS #9328
Conversation
plugins/enigma/enigma.js
Outdated
@@ -199,16 +200,19 @@ rcube_webmail.prototype.enigma_key_create_save = function () { | |||
// validate the form | |||
if (!password || !confirm) { | |||
this.alert_dialog(this.get_label('enigma.formerror')); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't do this for PHP and I don't like it here too.
}, | ||
{ | ||
'class': 'export', | ||
class: 'export', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid that this will not work. 'class' is a keyword in javascript.
EDIT: Hmm.. but the browser tests pass, so maybe it was an issue in some older browsers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
citing https://eslint.org/docs/latest/rules/quote-props
There are, however, some occasions when you must use quotes:
- If you are using an ECMAScript 3 JavaScript engine (such as IE8) and you want to use a keyword (such as if) as a property name. This restriction was removed in ECMAScript 5.
- You want to use a non-identifier character in your property name, such as having a property with a space like "one two".
https://caniuse.com/es5 it should be fine to assume ECMAScript 5 support
@@ -532,7 +543,7 @@ rcube_webmail.prototype.enigma_add_list_row = function (r) { | |||
|
|||
|
|||
/*********************************************************/ | |||
/********* Enigma Message methods *********/ | |||
/* ******** Enigma Message methods *********/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a rule like in php, it should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That added space does not make sense. Look at the line above (with all asterisks). If we can't fix it we might consider removing these whole comment blocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is f9b3ba6
IMO the comments can be helpful to place a new methods more logically as the files itself are quite large.
We did the same with PHP code, we simply made the fixer happy.
Thus I prefer to merge this PR as is.
regex: (^ *(break|return).*)\n *(\n)
9f3c924
to
c736a51
Compare
no functional change