Skip to content
This repository has been archived by the owner on May 7, 2024. It is now read-only.

Feature init #1

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Feature init #1

wants to merge 17 commits into from

Conversation

perruche
Copy link

@perruche perruche commented Oct 11, 2021

This PR contain a working integration to create ACFGroup
It is still a WIP considering architecture should be improve

How to test

  • Add this repository
  • run php merlin acf:setup to setup the absolute path in which files will be created
  • run php merlin acf:create to create a new field group by following the prompt

Todo

PHP tooling

  • Add PHPCS (PSR12)
  • Add PHPStan
  • Add PHPUnit

Command

  • Add acf:setup command to store the filepath that will be use to create the files with other commands
  • Add stub files
  • Replace stub file content with user input from the command
  • Generate the file

Issues / todo

  • Update a parent file that manage all the ACFGroups in a single class ?
  • Add two different PHPCS ? for stub files who are supposed to be WPCS compliant
  • Add handling a file to load all the field groups
  • Add tests

Added

  • php merlin acf:create command
  • php merlin acf:setup command
  • PHPStan
  • PHPCS
  • PHPUnit

@perruche perruche requested a review from a team October 11, 2021 14:08
@perruche perruche self-assigned this Oct 11, 2021
@perruche
Copy link
Author

Hey! Any update on the rest of the TODO list ?

@perruche perruche changed the title [WIP] Feature init Feature init Aug 1, 2022

## Installation
> **Requires PHP7.3**

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Add a specific section for requirements

- help
- list
- acf:create

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Only specify merlin list command to prevent to maintain this section up to date each time a command is added to the project

*/
protected function interact(InputInterface $input, OutputInterface $output): void
{
// @todo add a verification of the content of config file, else ask user to run acf:setup

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: If think it's a must have before merging this PR


// Field group location location.
$this->io->section('2. Field Group location');
$this->io->note('Exemple: post_type == product');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chore: Exemple -> Example

$this->io->section('1. Field Group informations');

// @todo add a validator to only allow lowercase, trim space, etc
$this->data['slug'] = $this->io->ask('Enter the name of the field group to create');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Force input style thanks to Symfony\Component\String\u function?


if ($confirm) {
$this->prepareFile();
// @todo Add creation of the templates here, use $this->data

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Todo seems to already be done. to remove

@@ -0,0 +1,46 @@
#$ composer update --optimize-autoloader

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Remove commented lines

@@ -0,0 +1,15 @@
<?php

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this file as it does not have real tests

@lusimeon
Copy link

lusimeon commented Aug 10, 2022

General feedback :

  • chore: Add linters and tests to Github actions
  • nitpick: Rename app folder to src to follow conventionnal structure.
  • nitpick: Add config/config.yml to .gitignore to prevent git following changes when running acf:setup. This file is very specific to running context.

@lusimeon
Copy link

lusimeon commented Aug 10, 2022

My opinion about PR todolist

Update a parent file that manage all the ACFGroups in a single class ?

This package should not load groups, only create it. Maybe we should force a specific path for groups to be able to autoload theme in studiometa/create-wordpress-project

Add two different PHPCS ? for stub files who are supposed to be WPCS compliant

+1, you can specify specific rules by folder thanks to phpcs.xml

Add handling a file to load all the field groups

This package should not load groups, only create it

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants