Page MenuHomePhabricator

Re-enable codesniffer sniffs disabled in MobileFrontend
Closed, ResolvedPublic

Description

PHPCodeSniffer helps us stick to same coding standards across MediaWiki and its extensions. Currently MobileFrontend phpcs config differs from the MediaWiki one - the following sniffs are disabled:

  • MediaWiki.NamingConventions.LowerCamelFunctionsName.FunctionName
  • MediaWiki.WhiteSpace.SpaceBeforeSingleLineComment.NewLineComment
  • MediaWiki.Usage.ExtendClassUsage.FunctionVarUsage
  • MediaWiki.Commenting.FunctionComment
  • MediaWiki.Files.ClassMatchesFilename.NotMatch" />
  • MediaWiki.Files.OneClassPerFile.MultipleFound" />
  • MediaWiki.WhiteSpace.SpaceBeforeSingleLineComment.NewLineComment
  • MediaWiki.Usage.ExtendClassUsage.FunctionVarUsage

Google Code In

@Jdlrobson will mentor this.
The task should be timeboxed till 2-3hrs - work on re-enabling one sniff at a time.

To get setup install https://github.com/squizlabs/PHP_CodeSniffer globally and then run the following inside the MobileFrontend repo

phpcs -p -s

They should all pass as we are excluding certain rules inside .phpcs.xml

Runing phpcs without all those exclusions gives 18 errors and 52 warnings.

Plan (YMMV)

  • Remove a single sniff from the .phpcs.xml file (an exclude tag), run composer phpcs or phpcs -p -s, get a list of violations, provide patches in Wikimedia Gerrit to fix the violations. If you fix all violations for one sniff, also commit the removal of the exclusion from .phpcs.xml too.
  • Commit the change when everything passes
  • Repeat until you have spent more than 3 hours on this task. You may be able to fix them all in this timebox, but you should not feel bad if you don't!

AC

  • As many of the sniffs are made to pass as is possible.
  • If a sniff can't be made to pass for some reason, then document it as close as possible to the line disabling the sniff.

Developer notes

CodeSniffer output: P5742

Event Timeline

pmiazga renamed this task from Fix codesniffer sniffs disabled in MobileFrontend to Re-enable codesniffer sniffs disabled in MobileFrontend.Jul 13 2017, 2:37 PM

@Jdlrobson: Any guestimation into how many Google-Code-in-2017 tasks this ticket should be broken down ("about 2-3 hours for an experienced contributor") or how many issues to fix?

Thanks. Published as https://codein.withgoogle.com/tasks/5449769772122112/ - might need another instance / copy if exceeding time.

Change 394324 had a related patch set uploaded (by AntoniSiek; owner: AntoniSiek):
[mediawiki/extensions/MobileFrontend@master] This commit partially fixes excludes from MobileFrontent extension

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

Looks like student ran out of time :( @Aklapper what should we do with the patch now? What's the process here?

@Jdlrobson, maybe extend the time for the student to complete the work or maybe @Aklapper has more ideas.

Change 396296 had a related patch set uploaded (by Albert221; owner: Albert221):
[mediawiki/extensions/MobileFrontend@master] Fix phpcs issues and remove its <exclude>s

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

Change 396296 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Fix phpcs issues and remove its <exclude>s

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

Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
Jdlrobson removed a project: Patch-For-Review.
Jdlrobson added a subscriber: Albert221.

@Aklapper you can re-import the remaining work as a new task now - I've updated the description.

Looks like student ran out of time :( @Aklapper what should we do with the patch now? What's the process here?

Uhm, sorry for my late reply. "Add time" is available for mentors on the GCI site but (I think?) only before the student hits the deadline. :-/

@Aklapper you can re-import the remaining work as a new task now - I've updated the description.

Done: https://codein.withgoogle.com/tasks/6143358965645312/

Change 398075 had a related patch set uploaded (by Rafidaslam; owner: Rafid Aslam):
[mediawiki/extensions/MobileFrontend@master] Re-enable MediaWiki.Usage.ExtendClassUsage.FunctionVarUsage sniff

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

Change 398076 had a related patch set uploaded (by Rafidaslam; owner: Rafid Aslam):
[mediawiki/extensions/MobileFrontend@master] Enable MediaWiki.WhiteSpace.SpaceBeforeSingleLineComment.NewLineComment

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

Change 398076 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Enable MediaWiki.WhiteSpace.SpaceBeforeSingleLineComment.NewLineComment

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

Change 398075 abandoned by Rafidaslam:
Re-enable MediaWiki.Usage.ExtendClassUsage.FunctionVarUsage sniff

Reason:
As explained on IRC, per T179753 this sniff is broken and buggy and should stay disabled. (Legoktm)

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

Change 400412 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/MobileFrontend@master] Remove superfluous documentation snippets

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

Change 401625 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Fix and restrict sniffs to certain files

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

Change 400412 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove superfluous documentation snippets

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

https://gerrit.wikimedia.org/r/401625 is a follow up to @thiemowmde 's changes - I'd love to get that reviewed so we can close this out.

Jdlrobson raised the priority of this task from Low to Medium.Jan 8 2018, 9:57 PM
Jdlrobson moved this task from Triaged but Future to Upcoming on the Web-Team-Backlog board.

Change 401625 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Fix and restrict sniffs to certain files

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

Change 403941 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Remove exclusion in phpcs

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

Jdlrobson updated the task description. (Show Details)

Change 403941 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove exclusion in phpcs; lower severity

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

Change 394324 abandoned by Jdlrobson:
This commit partially fixes excludes from MobileFrontent extension

Reason:
Old patchset. Problems have since been fixed.

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