Page MenuHomePhabricator

[SPIKE 16hrs] Add coding convention for new commits to Vector
Closed, ResolvedPublic3 Estimated Story PointsSpike

Description

Vector is a tiny codebase currently (as compared to other repositories Web stewards, especially MobileFrontend and MinervaNeue). As such, it's ours to lose. If we merge YOLO "hero" commits that scramble the codebase, accept "rockstar" hackathon patches that add months of technical debt, or otherwise +2 patches that violate presently implicit standards, we'll never be able to recover and will succeed only in recreating our mistakes from previous projects. This should be well within our realm of influence even with tight schedules. Where automated tests cannot be employed, agreed and formally documented contribution standards in the repo will help us recognize the shortcomings in these patches and point others to them for easy identification.

This task encompasses the work to set some basic, uncontroversial coding standards in writing as part of the Vector readme file. For example, coding guidelines around adding unit tests, keeping documentation current, etc. MobileFrontend presently requires a threshold for unit tests but, regardless, agreeing on the principle in the conventions is important too. Broadly, I hope that we can agree that patches will be iterated on until they're of great quality so that incomplete patches don't slip through to master and mistakes from other projects are repeated. In a nutshell, don’t write bad code and certainly don’t merge it.

It should be fast to iterate on conventions and prove their usefulness within a single project, Vector. Most of these lessons, once polished and proven, can then be migrated to broader coding standards across repositories at the Foundation.

For automating some norms, @phuedx mentioned Danger JS as a possibility.

AC

QA

No functional changes so no QA needed.

Event Timeline

Restricted Application changed the subtype of this task from "Task" to "Spike". · View Herald TranscriptNov 26 2019, 7:09 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Isarra added a subscriber: Isarra.Nov 27 2019, 9:02 PM

Vector is a tiny codebase currently.

...compared to what? For a skin, it's already huge, unless you're comparing to third-party skins such as wikiHow's Owl or Fandom's Oasis skins, or Minerva, which didn't even begin as a skin at all. That's not really going to be a fair comparison anyway as the former are both single-skin sites that include quite a bit of specific functionality and extra extension handling in the skins themselves, and the later likewise existing as essentially its own version of the site. This has, for Minerva, led to most of the present work on it to consist of trying to get this stuff out of the skin, as it makes it extremely difficult to update and maintain, and for wikiHow and Fandom, been a major contributing factor to why they so rarely update their mw version at all.

Niedzielski updated the task description. (Show Details)Dec 2 2019, 6:58 PM
Restricted Application added a subscriber: Masumrezarock100. · View Herald TranscriptDec 2 2019, 6:58 PM
Niedzielski updated the task description. (Show Details)Dec 2 2019, 8:22 PM

This task encompasses the work to set some basic, uncontroversial coding standards in writing as part of the Vector readme file. For example, coding guidelines around adding unit tests, keeping documentation current, etc.

@Niedzielski How is this to be understood differently to general coding guidelines for all Wikimedia projects? To me the task description as is reflects foremost coding conventions and guidelines, that we, as Foundation employees and volunteer contributors are supposed to aim for anyways…

@Volker_E, for better and worse it's been my experience that the Web team is often pioneering new technologies, and coding patterns and techniques that occasionally percolate out to the broader Foundation's repositories. Having a universal coding convention (beyond linting) sounds appealing, but in practice, I think repositories are often focused on very different problems and conventions that seem like no-brainers in one place may be entirely impractical and be a negative force on development in another. Additionally, assigning a team all the responsibilities of maintaining and significantly extending a project but demanding that all coding conventions pass through the gatekeepers of the conventions for all repositories under the Foundation's purview is not empowering and leads to lazy contributions in my perspective. I would rather see the team fully embrace a convention that makes sense to them for a given project.

For these reasons, a universal coding convention that applies everywhere does not make much sense, in my opinion, and specific per repo conventions are preferable when a team is assigned a long term project investment in it. This is the case for the Desktop Improvements Project.

Niedzielski updated the task description. (Show Details)Dec 4 2019, 6:35 PM
Niedzielski updated the task description. (Show Details)Dec 6 2019, 3:23 PM
Volker_E updated the task description. (Show Details)Dec 11 2019, 7:01 PM
ovasileva triaged this task as Medium priority.Jan 6 2020, 4:11 PM
ovasileva set the point value for this task to 3.Jan 8 2020, 6:32 PM
Niedzielski updated the task description. (Show Details)Jan 8 2020, 10:49 PM
Niedzielski added a subscriber: phuedx.
ovasileva renamed this task from Add coding convention for new commits to Vector to [SPIKE 12HRS] Add coding convention for new commits to Vector.Jan 9 2020, 6:18 PM
ovasileva renamed this task from [SPIKE 12HRS] Add coding convention for new commits to Vector to [SPIKE 16hrs] Add coding convention for new commits to Vector.
Jdlrobson moved this task from Needs triage to Technical on the Vector board.Jan 15 2020, 5:57 PM

Change 566639 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/Vector@master] POC: Using Danger.js to codify conventions

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

Change 566885 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/Vector@master] Add coding conentions to Vector

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

Change 566885 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Add Coding Conventions Section to Vector README

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

Change 570761 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/Vector@master] POC: Use shared eslint web team rules

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

Change 570952 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/MinervaNeue@master] Remove eslint "valid-jsdoc" rules/fix linting errors

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

Change 570952 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Remove eslint "valid-jsdoc" rules/fix linting errors

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

Hey @nray! Is there anything else that needs review?

Change 570761 abandoned by Nray:
POC: Use shared eslint web team rules

Reason:
See comment above

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

Change 566639 abandoned by Nray:
POC: Using Danger.js to codify conventions

Reason:
Relative to our work, I think Danger's git API is most useful for enforcing directory/file naming/location conventions but until https://github.com/danger/danger-js/issues/972 is resolved, I'm not recommending it at this time

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

nray added a comment.Feb 11 2020, 5:47 PM

@phuedx Not anymore, I think this is ready for sign off now, There are still some topics I'd like to discuss in our next shdt whenever that may be, but I don't think that should block the resolution of this task.

nray removed nray as the assignee of this task.Feb 11 2020, 5:47 PM
nray added a subscriber: nray.

Change 571588 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/MinervaNeue@master] Prevent our 'no-restricted-properties' from clobbering eslint-config-wikimedia

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

nray added a comment.Feb 11 2020, 11:07 PM

Okay, this is probably a bit of scope creep now but there actually is one more patch I'd like to get merged before this task is resolved:

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

It just fixes a bit of Minerva's eslint rules which I'm sure we will take inspiration from when setting up vector eslint

Maybe @Niedzielski can review this as part of sign off (or feel free to move back to needs code review column) :)

Niedzielski closed this task as Resolved.Feb 12 2020, 3:48 PM

Change 571588 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Prevent our 'no-restricted-properties' from clobbering eslint-config-wikimedia

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