238 phpcs scaffold #562
238 phpcs scaffold #562
Conversation
| "[project-root]/phpcs.xml": "scaffold/phpcs.xml" | ||
| "[project-root]/phpcs.xml.dist": "scaffold/phpcs.xml.dist" |
There was a problem hiding this comment.
Have we renamed an existing scaffolding file before? We have encountered errors with this on previous projects when the root project ends up having any kind of dependency or sub-dependency on composer/composer: https://www.drupal.org/project/drupal/issues/3266399
I'd like to make sure this is tested with a manual composer update to confirm it's okay to rename. It might be safer to leave the existing file in the source repository here and essentially have duplicate files, but only one listed in the scaffolding itself.
There was a problem hiding this comment.
Took me a bit of time to figure out how to test with the existing test framework. I have tested out what happens when the upgrade happens. I have tested upgrading and all it seems to do is create a phpcs.xml.dist file. Not seeing any errors.
tasks/test.yml
Outdated
| @@ -137,7 +137,7 @@ tasks: | |||
| desc: Runs PHPCS with Drupal Coding Standards | |||
| summary: | | |||
| Check your code against Drupal's coding standards. To override the default | |||
| ruleset, copy vendor/lullabot/drainpipe/scaffold/phpcs.xml to phpcs.xml | |||
| ruleset, copy vendor/lullabot/drainpipe/scaffold/phpcs.xml.dist to phpcs.xml.dist | |||
There was a problem hiding this comment.
| ruleset, copy vendor/lullabot/drainpipe/scaffold/phpcs.xml.dist to phpcs.xml.dist | |
| ruleset, copy vendor/lullabot/drainpipe/scaffold/phpcs.xml.dist to phpcs.xml |
There was a problem hiding this comment.
I don't understand this suggestion.
There was a problem hiding this comment.
The instructions are to copy the file to the same name?
There was a problem hiding this comment.
It is the same pattern that was there before I made the change. All I did was change the file name.
There was a problem hiding this comment.
I think this is a bit confusing, because .dist usually means "shipped with the project" not "local edits". I think phpcs.xml would be more canonical... but we should probably also automatically copy phpcs.xml.dist to the root of the repo?
There was a problem hiding this comment.
I'm confused I thought the whole point of the ticket was to have drainpipe generate the projects phpcs.xml.dist instead of the phpcs.xml
There was a problem hiding this comment.
I guess I'm confused because the instruction says "To override the default ruleset" in the beginning, which to mean assumes this is for a project-specific config file. I would assume the scaffolding file is being copied to the root of the project, which is what I see based on "[project-root]/phpcs.xml.dist": "scaffold/phpcs.xml.dist" being in the composer.json...
There was a problem hiding this comment.
Oh, I see... ya I don't know where my brain was at that day. Maybe that should be changed to direct the user to the readme because I don't see these instructions fitting into that.
| @@ -681,6 +667,10 @@ Peer Reviewing by looking at PR code changes is nice. | |||
|
|
|||
| Testing PR code changes on real sites is extra beneficial. | |||
|
|
|||
| ### Local Development | |||
There was a problem hiding this comment.
A new heading, you'll need to run the command to update the table of contents in the file. We should really have a GitHub workflow that fails if this hasn't been updated.
There was a problem hiding this comment.
It would be kind of neat if github could rerun doctoc and then push a new commit. I can imagine other tools where we'd want to do something like that too.
There was a problem hiding this comment.
I updated the toc. Looks like #567 covers the suggested improvements.
Fixes #238
Documentation updates and makes drainpipe generate a phpcs.xml.dist instead of a phpcs.xml