open-lms-open-source / moodle-plugin-ci

Assist with running a Moodle plugin in Travis CI
https://blackboard-open-source.github.io/moodle-plugin-ci/
GNU General Public License v3.0
43 stars 37 forks source link

Add "Javascript coding style problems" from moodle.org/plugins prechecker to moodle-plugin-ci #95

Open abias opened 5 years ago

abias commented 5 years ago

Hi,

I have noticed that the moodle.org/plugins prechecker runs some check on the Javascript Coding style which are currently not reported by moodle-plugin-ci.

For an example, please see https://moodle.org/plugins/pluginversion.php?id=19343&smurf=html#js where this error is reported:

lib/editor/atto/plugins/styles/yui/src/button/js/button.js
(#37) Expected space or tab after '/*' in comment. (spaced-comment)

At the same time, the corresponding Job in Travis CI does not report any problems with JavaScript: https://travis-ci.org/moodleuulm/moodle-atto_styles/jobs/518201841

Do you have an idea if these JavaScript checks can also be added to moodle-plugin-ci?

Cheers, Alex

abias commented 4 years ago

Hi @polothy ,

may I ping you if you have an idea about this gap?

Thanks, Alex

polothy commented 4 years ago

Hrm... I don't know what the js check is. IIRC, it would be defined somewhere in this project: https://github.com/moodlehq/moodle-local_ci

abias commented 4 years ago

Thank you @polothy .

I have raised this question in Moodle Dev Chat to see if anyone from Moodle HQ can shed a light on this architecture.

andrewnicols commented 4 years ago

It's an eslint warning which matches https://eslint.org/docs/rules/spaced-comment

The line in question is:

/*global rangy*/

You can fix this in several ways:

  1. Switch from rangy to window.rangy - this is the one that you should do. This is more correct. rangy is just a shortcut to window.rangy. The rangy option works because it's the default scope for when an object is not properly scoped.
  2. Switch the line to read /* global rangy */

It's a valid issue, not a bug in the checker. You can see the same thing by running eslint manually:

npx eslint lib/editor/atto/plugins/styles/yui/src/button/js/button.js
abias commented 4 years ago

Thanks, @andrewnicols , for your quick feedback about the underlying coding style problem. I will use your improvement proposal soon to improve the code.

However, my question was about the fact that this coding style problem is reported by the moodle.org/plugins prechecker but not by Travis CI which is running local_codechecker.

@polothy was wondering if these eslint checks are only performed by moodle-local_ci in the Moodle plugins repository.

Do you also have any insights about that for me?

Thanks, Alex

abias commented 4 years ago

Hey @andrewnicols , may I ping you again about the pending question from my comment above?

@polothy was wondering if these eslint checks are only performed by moodle-local_ci in the Moodle plugins repository.

Thanks, Alex

marinaglancy commented 4 years ago

hi! I added an option to run grunt with arguments to show/fail on lint warnings, see https://github.com/blackboard-open-source/moodle-plugin-ci/pull/111 it is already merged but not tagged yet