a8cteam51 / team51-cli

1 stars 1 forks source link

Migrate `pattern-export-to-repo` command #29

Closed GeoJunkie closed 4 months ago

GeoJunkie commented 7 months ago

This is the first part of #6 .

Migrates the pattern-export-to-repo command from the old CLI tool.

The command should not be launched until the image replacement updates are complete (https://github.com/a8cteam51/team51-cli/issues/6#issuecomment-2069454964), but this needs a review to make sure it fits with the new architecture.

ahegyes commented 7 months ago

I typically recommend the following naming structure:

This keeps commands related to each other sorted in the IDE (all DeployHQ commands are "together", then all "DeployHQ project" commands, then all "DeployHQ project server" commands, and so on...).

At the same time, it's more intuitive to type in a command that starts with a verb (you type in what you're doing, then press tab for options).

Based on that, I would rename the file GitHub_Pattern_To_Repo_Export and the command to github:export-pattern-to-repo. But this is merely a suggestion ... If you think the current names make more sense for this particular case, cool 😄

ahegyes commented 7 months ago

After reviewing what the command itself does, I do have a philosophical suggestion -- what if we split the command in two? Technically, each command is supposed to "do one thing on one service". If we need multiple things done, then we should chain the commands.

That way, if anything fails, we can easily continue the process by calling the command that failed with the appropriate arguments. Look at the pressable:site-create command which will now make calls to deployhq:create-project and deployhq:create-project-server. This way, if creating the DeployHQ stuff fails for whatever reason, we only need to call those commands instead of doing everything manually.

Moreover, maybe there will be a legitimate use-case of exporting the WP Pattern and doing something else with it, not just uploading it to GitHub. Or, maybe we want to reuse the upload to GitHub part on sites not hosted on Pressable.

So I would suggest these two commands:

1) pressable:export-site-pattern hosted in Pressable_Site_Pattern_Export.php (later we can then create a command called wpcom:export-site-pattern hosted in WPCOM_Site_Pattern_Export.php)

2) github:commit-site-pattern hoted in GitHub_Site_Pattern_Commit.php

Thoughts?

GeoJunkie commented 7 months ago

I typically recommend the following naming structure:

  • file: <service>_<object>_<descriptors>_<action>
  • command: <service>:<action>_<object>_<descriptors>

This keeps commands related to each other sorted in the IDE (all DeployHQ commands are "together", then all "DeployHQ project" commands, then all "DeployHQ project server" commands, and so on...).

At the same time, it's more intuitive to type in a command that starts with a verb (you type in what you're doing, then press tab for options).

Based on that, I would rename the file GitHub_Pattern_To_Repo_Export and the command to github:export-pattern-to-repo. But this is merely a suggestion ... If you think the current names make more sense for this particular case, cool 😄

I was basing this on the old setup where the command name and the filename matched. This makes sense, so I updated it :). (Commit d91df5b )

GeoJunkie commented 7 months ago

After reviewing what the command itself does, I do have a philosophical suggestion -- what if we split the command in two? Technically, each command is supposed to "do one thing on one service". If we need multiple things done, then we should chain the commands.

That way, if anything fails, we can easily continue the process by calling the command that failed with the appropriate arguments. Look at the pressable:site-create command which will now make calls to deployhq:create-project and deployhq:create-project-server. This way, if creating the DeployHQ stuff fails for whatever reason, we only need to call those commands instead of doing everything manually.

Moreover, maybe there will be a legitimate use-case of exporting the WP Pattern and doing something else with it, not just uploading it to GitHub. Or, maybe we want to reuse the upload to GitHub part on sites not hosted on Pressable.

So I would suggest these two commands:

  1. pressable:export-site-pattern hosted in Pressable_Site_Pattern_Export.php (later we can then create a command called wpcom:export-site-pattern hosted in WPCOM_Site_Pattern_Export.php)
  2. github:commit-site-pattern hoted in GitHub_Site_Pattern_Commit.php

Thoughts?

@ahegyes I agree in part. I was thinking about splitting it up while I was migrating it but had initially decided against it. Give the "do one thing on one service" philosophy, it makes sense to have the functions of the command split up, but I also think having one command to carry out all the steps in the most common process makes a lot of sense here, too. We'll be asking TAMs to do this task a lot, and if they can do it in one step instead of two that would be better.

Can we do something similar to pressable:site-create where one service command calls the others?

I'm thinking we would have pressable:export-pattern-to-library (with the potential to add wpcom:export-pattern-to-library later). This command would then chain the two commands: pressable:export-site-pattern and github:add-pattern, and pass the exported file from one to the other.

I renamed the commands slightly to cover what they would do:

What do you think?

ahegyes commented 7 months ago

What's the purpose of storing that back in the argument and setting it as a class property?

If the argument is marked as mandatory, Symfony will exit the command if it's not set. We can set it programmatically in the initialize method so that's what we do. This is not required if the argument is optional but I like to do it always because it keeps the app status consistent.

As for keeping it a class property ... well, you're right, we could technically fetch it always from the input arguments. I think that I actually do that in 1-2 commands when I just needed it for an input helper ... but it's more prone to typos and it doesn't have IDE auto-complete features.

I hadn't given this too much though beyond always setting it on $input to keep Symfony happy 😅 Maybe we should reuse that more often ...

ahegyes commented 7 months ago

Can we do something similar to pressable:site-create where one service command calls the others?

Absolutely, and that's what I meant! We could add a negatable option (InputOption::VALUE_NEGATABLE, see https://symfony.com/doc/current/console/input.html) like --commit-to-github which would then chain the commands (or not ... not sure whether the default should be yes or no).

I renamed the commands slightly to cover what they would do: [...] What do you think?

I don't think we really need 3 commands. We can just do this:

  1. github:commit-pattern (or github:commit-gutenberg-pattern or github:commit-gb-pattern or github:commit-wp-pattern, really not sure what my favorite is ... thoughts?)
  2. pressable:export-site-pattern with the negatable option --commit-to-github in order to chain or not chain the GH command; as said above, not sure if --commit or --no-commit should be default
GeoJunkie commented 6 months ago

What's the purpose of storing that back in the argument and setting it as a class property?

If the argument is marked as mandatory, Symfony will exit the command if it's not set. We can set it programmatically in the initialize method so that's what we do. This is not required if the argument is optional but I like to do it always because it keeps the app status consistent.

As for keeping it a class property ... well, you're right, we could technically fetch it always from the input arguments. I think that I actually do that in 1-2 commands when I just needed it for an input helper ... but it's more prone to typos and it doesn't have IDE auto-complete features.

I hadn't given this too much though beyond always setting it on $input to keep Symfony happy 😅 Maybe we should reuse that more often ...

This all makes sense! For now, I'll leave it as-is. As things update we can use it more :D