Page MenuHomePhabricator

MediaViewer: Various cleanup patches
Closed, ResolvedPublic

Description

This card tracks a few cleanup/maintenance patches unrelated to specific tasks or bugs.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 22 2020, 10:29 PM
Demian triaged this task as Medium priority.Feb 22 2020, 10:29 PM

Change 574202 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/extensions/MultimediaViewer@master] Maintenance: eslint (run with: grunt test) will automatically fix trivial coding style errors

https://gerrit.wikimedia.org/r/574202

Change 574203 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/extensions/MultimediaViewer@master] Maintenance: grunt test won't run the unnecessary 'svgmin' task. grunt (default) will, though.

https://gerrit.wikimedia.org/r/574203

Demian moved this task from Backlog to In Process on the MediaViewer board.

Change 574223 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/extensions/MultimediaViewer@master] Maintenance: add grunt lint and grunt minify, consistent with core, and grunt build, npm build, remove grunt default

https://gerrit.wikimedia.org/r/574223

Demian added a subscriber: Esanders.EditedFeb 23 2020, 3:05 PM

@Esanders wrote on gerrit:
Oh I see you added this in this commit. We don't run 'fix' by default, some fixes (especially whitespace fixes) require manual checking, so shouldn't be run without the user knowing about it. If we want to change this convention we should have a separate discussion about it.

@Demian wrote:
Yes, it saved me maybe hours. It's a good point tho to make the user aware. Can we do something about that, maybe ask the user interactively?
What whitespace fix needs manual checking? Can you give an example?

I found the fix option very useful, unfortunately it's not possible to specify on the command line afaict. A separate task, maybe 'lintfix' or 'fix' (or sg short) could do that, so the user requests it explicitly.

In the past we have sometimes created a subtask for fix, then you can grunt eslint:fix but it might be nicer to use grunt.option('fix'):

Gruntfile.js
eslint: {
  options: {
    fix: grunt.option('fix')
  }
}

then
grunt eslint --fix

Another issue I just noticed is that if you run fix as part of default then it will get run by default by CI and errors will not get detected.

Another issue I just noticed is that if you run fix as part of default then it will get run by default by CI and errors will not get detected.

I've removed that from the chain as WIP, so the rest is not stalled.

Change 574486 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/extensions/MultimediaViewer@master] Maintenance: grunt jsfix, grunt cssfix and their shorthands grunt jf, grunt cf automatically fixes trivial coding style errors

https://gerrit.wikimedia.org/r/574486

Change 574490 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/extensions/MultimediaViewer@master] Maintenance: grunt stylelint --fix added a 2 spaces

https://gerrit.wikimedia.org/r/574490

Change 574203 merged by jenkins-bot:
[mediawiki/extensions/MultimediaViewer@master] Maintenance: grunt test won't run the unnecessary 'svgmin' task. grunt (default) will, though.

https://gerrit.wikimedia.org/r/574203

Change 574223 merged by jenkins-bot:
[mediawiki/extensions/MultimediaViewer@master] Maintenance: add grunt lint and grunt minify, consistent with core, and grunt build, npm build

https://gerrit.wikimedia.org/r/574223

Change 574202 merged by jenkins-bot:
[mediawiki/extensions/MultimediaViewer@master] Maintenance: grunt lint --fix, grunt eslint --fix, grunt stylelint --fix automatically fixes trivial coding style errors

https://gerrit.wikimedia.org/r/574202

@Esanders I've submitted a POC patch in T242781 that makes it possible to delete the 40+ MiB node_modules (including grunt) from the project folder.

Per a comment on gerrit, an example where auto-fixing needs manual review is the auto-indenting of block comments:

Before fix
( function () {
/**
 * Check if this thumbnail should be handled by MediaViewer
 *
 * @param {jQuery} $thumb the thumbnail (an `<img>` element) in question
 * @return {boolean}
 */
MMVB.isAllowedThumb = function ( $thumb ) {
	...
};
}() );
After fix
( function () {
	/**
 * Check if this thumbnail should be handled by MediaViewer
 *
 * @param {jQuery} $thumb the thumbnail (an `<img>` element) in question
 * @return {boolean}
 */
	MMVB.isAllowedThumb = function ( $thumb ) {
		...
	};
}() );

Good to know, I'll pay attention to that. Thank you.

Change 574636 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/extensions/MultimediaViewer@master] Replace grunt tasks with npm scripts, remove unnecessary devDependencies.

https://gerrit.wikimedia.org/r/574636

Change 574490 merged by jenkins-bot:
[mediawiki/extensions/MultimediaViewer@master] Maintenance: npm -s run lint:fix caught some missed empty lines and spaces.

https://gerrit.wikimedia.org/r/574490

Change 575396 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/extensions/MultimediaViewer@master] Regenerate 'package-lock.json'

https://gerrit.wikimedia.org/r/575396

Change 575664 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/extensions/MultimediaViewer@master] Update node_modules dependencies in package-lock.json

https://gerrit.wikimedia.org/r/575664

Change 575665 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/extensions/MultimediaViewer@master] Replace grunt* devdependencies with eslint, update node_modules dependencies in package-lock.json

https://gerrit.wikimedia.org/r/575665

Change 575666 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/extensions/MultimediaViewer@master] Regenerate package-lock.json

https://gerrit.wikimedia.org/r/575666

Change 575668 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/extensions/MultimediaViewer@master] Update node_modules dependencies in package-lock.json after deleting node_modules

https://gerrit.wikimedia.org/r/575668

Demian renamed this task from MultiMediaViewer: Various cleanup patches (tracking task) to MediaViewer: Various cleanup patches.Jul 4 2020, 12:16 AM
Demian closed this task as Resolved.Nov 2 2020, 1:54 AM
Demian removed a project: Patch-For-Review.

No more to do for the time being.

Change 637820 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/extensions/MultimediaViewer@master] [cleanup] Fix eslint (new version) warnings

https://gerrit.wikimedia.org/r/637820

Change 637820 merged by jenkins-bot:
[mediawiki/extensions/MultimediaViewer@master] [cleanup] Fix eslint (new version) warnings

https://gerrit.wikimedia.org/r/637820

Demian closed this task as Resolved.Nov 2 2020, 3:03 AM
Demian removed a project: Patch-For-Review.
Demian added a subscriber: DannyS712.

Aaaand... it's gone. Thank you, @DannyS712!