Skip to content
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

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

Open
abias opened this issue Jun 12, 2019 · 7 comments

Comments

@abias
Copy link

abias commented Jun 12, 2019

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
Copy link
Author

abias commented Nov 29, 2019

Hi @polothy ,

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

Thanks,
Alex

@polothy
Copy link
Contributor

polothy commented Dec 2, 2019

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
Copy link
Author

abias commented Dec 4, 2019

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
Copy link

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
Copy link
Author

abias commented Dec 4, 2019

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
Copy link
Author

abias commented Feb 13, 2020

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
Copy link
Contributor

hi! I added an option to run grunt with arguments to show/fail on lint warnings, see #111
it is already merged but not tagged yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants