Project

General

Profile

Actions

Task #86896

closed

Apply CGL consistently in .yaml files

Added by Sybille Peters over 5 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
Code Cleanup
Target version:
-
Start date:
2018-11-09
Due date:
% Done:

100%

Estimated time:
TYPO3 Version:
9
PHP Version:
Tags:
Complexity:
no-brainer
Sprint Focus:
Remote Sprint

Description

The CGL page for Yaml files has been added: https://docs.typo3.org/typo3cms/CoreApiReference/CodingGuidelines/CglYaml.html

The indenting with 2 spaces appears to be applied consistently in the core, but using double quotes ("") or ('') for strings is not.

double quotes:

  • rte_ckeditor

single quotes:

  • sites: generated config.yaml
  • form
  • typo3/sysext/frontend/Tests/Functional/SiteHandling/Fixtures/SlugScenario.yaml
  • ... all others I checked.
Actions #1

Updated by Sybille Peters over 5 years ago

  • Complexity set to no-brainer
Actions #2

Updated by Sybille Peters over 5 years ago

  • Category deleted (System/Bootstrap/Configuration)
Actions #3

Updated by Sybille Peters over 4 years ago

.yaml <> .yml

Also, there are a lot of .yaml files:

typo3/sysext/backend/Tests/Functional/Fixtures/CommonScenario.yaml
typo3/sysext/core/Tests/Unit/Configuration/Loader/Fixtures/Subfolder/BaseTestInclusion.yaml
typo3/sysext/core/Tests/Unit/Configuration/Loader/Fixtures/Berta.yaml
typo3/sysext/core/Tests/Unit/Configuration/Fixtures/SiteConfigs/config2.yaml
typo3/sysext/core/Tests/Unit/Configuration/Fixtures/SiteConfigs/include.yaml
typo3/sysext/core/Tests/Unit/Configuration/Fixtures/SiteConfigs/config1_expected.yaml
typo3/sysext/core/Tests/Unit/Configuration/Fixtures/SiteConfigs/config1.yaml
typo3/sysext/core/Tests/Unit/Configuration/Fixtures/SiteConfigs/config2_expected.yaml
typo3/sysext/core/Tests/Unit/DependencyInjection/Fixtures/Package3/Configuration/Services.yaml
typo3/sysext/core/Tests/Unit/DependencyInjection/Fixtures/Package1/Configuration/Services.yaml
typo3/sysext/core/Tests/Unit/DependencyInjection/Fixtures/Package4Cycle/Configuration/Services.yaml
typo3/sysext/core/Tests/Unit/DependencyInjection/Fixtures/Package2/Configuration/Services.yaml
typo3/sysext/core/Tests/Functional/Fixtures/Extensions/irre_tutorial/Configuration/ExtensionBuilder/settings.yaml
typo3/sysext/form/Tests/Unit/Mvc/Persistence/Fixtures/BlankForm.form.yaml
typo3/sysext/form/Tests/Unit/Mvc/Configuration/Fixtures/OverruleNonArrayValuesOverArrayValues2.yaml
typo3/sysext/form/Tests/Unit/Mvc/Configuration/Fixtures/OverruleNonArrayValuesOverArrayValues1.yaml
typo3/sysext/form/Tests/Unit/Mvc/Configuration/Fixtures/Header.yaml
typo3/sysext/form/Tests/Unit/Mvc/Configuration/Fixtures/Invalid.yaml
typo3/sysext/form/Tests/Unit/Controller/Fixtures/BlankForm.yaml
typo3/sysext/form/Tests/Unit/Controller/Fixtures/SimpleContactForm.yaml
typo3/sysext/form/Tests/Functional/Hooks/Fixtures/test_resources/Configuration/Yaml/AllowedExtensionPaths.yaml
typo3/sysext/form/Tests/Functional/Hooks/Fixtures/test_resources/Configuration/Form/updated.form.yaml
typo3/sysext/form/Tests/Functional/Hooks/Fixtures/test_resources/Configuration/Form/legacy.yaml
typo3/sysext/frontend/Tests/Functional/SiteHandling/Fixtures/SlugScenario.yaml
typo3/sysext/frontend/Tests/Functional/SiteHandling/Fixtures/PlainScenario.yaml
typo3/sysext/frontend/Tests/Functional/SiteHandling/LocalizedPageRendering/Fixtures/ScenarioC.yaml
typo3/sysext/frontend/Tests/Functional/SiteHandling/LocalizedPageRendering/Fixtures/ScenarioF.yaml
typo3/sysext/frontend/Tests/Functional/SiteHandling/LocalizedPageRendering/Fixtures/ScenarioB.yaml
typo3/sysext/frontend/Tests/Functional/SiteHandling/LocalizedPageRendering/Fixtures/ScenarioA.yaml
typo3/sysext/frontend/Tests/Functional/SiteHandling/LocalizedPageRendering/Fixtures/ScenarioD.yaml
typo3/sysext/frontend/Tests/Functional/SiteHandling/LocalizedPageRendering/Fixtures/ScenarioE.yaml

and a few .yml files:

typo3/sysext/seo/Tests/Functional/Fixtures/HrefLangScenario.yml
typo3/sysext/core/Tests/Acceptance/Install.suite.yml
typo3/sysext/core/Tests/Acceptance/Backend.suite.yml
typo3/sysext/core/Tests/codeception.yml

The CGL currently states .yaml as preferred (though in the field .yml seeems to be used dominantly, see also https://stackoverflow.com/questions/21059124/is-it-yaml-or-yml)

Actions #4

Updated by Christian Eßl about 4 years ago

  • Category set to Code Cleanup
Actions #5

Updated by Sybille Peters about 4 years ago

This might not really be doable, if some third party code expects .yml and we currently use .yaml. I noticed it is just in some of the Tests where .yml exists.

@Christian could you check this? If this is the case, we should probably close the issue.

Actions #6

Updated by Mathias Schreiber about 2 years ago

  • Sprint Focus set to Remote Sprint
Actions #7

Updated by Larry Garfield about 2 years ago

I poked around a bit here.

What's unclear to me from the current YAML CG page is if the intent is "if you're going to quote a string, favor single quotes" or "all strings must use single quotes, not bare strings". (YAML allows bare strings in most cases.)

I checked for various other projects, and essentially none have the second policy. There's one that says "quote everything, except for these common cases", which seems like a bad approach for us.

Assuming the intent is the former, we should clarify on the docs page.

By my IDE's count, there are 154 YAML files in core today, and 362 " marks. Some of them are definitely correct (either they are doing interpolation of some kind or they're an HTML string in a multi-line string). An automated solution is unlikely to be applicable here, so that means just a manual review to find excessive " marks.

I have also been unable to find any linting tools that can automate this process to integrate into CI, so we should expect some "style rot" here no matter what we do.

That leaves as my recommended action items:

  • Clarify on the docs page that we mean "if you're going to quote, use single quotes".
  • Manually grind through every " in the current YAML files and fix them if appropriate; at the current scale that will be annoying but doable in an hour or three.

Unless anyone objects, I will do the above two steps shortly.

Actions #8

Updated by Gerrit Code Review about 2 years ago

  • Status changed from New to Under Review

Patch set 1 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/74326

Actions #9

Updated by Gerrit Code Review about 2 years ago

Patch set 2 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/74326

Actions #10

Updated by Gerrit Code Review about 2 years ago

Patch set 3 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/74326

Actions #11

Updated by Larry Garfield almost 2 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100
Actions #12

Updated by Benni Mack over 1 year ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF