Skip to content
This repository has been archived by the owner on Aug 2, 2020. It is now read-only.

Split up remaining pieces in public/index.php #15

Closed
wants to merge 2 commits into from
Closed

Conversation

jwage
Copy link
Member

@jwage jwage commented Apr 13, 2019

Fixes #4

TODO:

  • get feedback from Marco on direction
  • I think maybe a few other things could be split out a little more
  • Add tests

{
public function __invoke(string $buildDir) : void
{
(new Process(['rm', '-rf', $buildDir]))
Copy link
Member

Choose a reason for hiding this comment

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

At this point, we need to assert that $buildDir is a directory, before performing something this scary :)

public function __invoke(
UriInterface $repositoryUri,
string $targetPath,
string $gitAuthorName,
Copy link
Member

Choose a reason for hiding this comment

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

These values should be asserted upon, or we should have value types introduced

public function __invoke(
string $repositoryDirectory,
BranchName $sourceBranch,
string $tagName,
Copy link
Member

Choose a reason for hiding this comment

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

We need to assert on the tag being a non-empty string

@Ocramius
Copy link
Member

get feedback from Marco on direction

As you can see from the structure of the code that was ported, it is extremely hard to test in isolation. That will be a pain, but we'll need to go through it. Also, I think that the various string parameters, especially when representing a Directory or a GitRepositoryRootPath, we should have value types representing them, reducing the risk of incorrect strings being passed around.

@jwage
Copy link
Member Author

jwage commented Apr 13, 2019

Cool. That is aligned with what I was thinking needs to happen next. I will keep working on it.

@Ocramius
Copy link
Member

Ocramius commented Aug 2, 2020

Closing here: this has been implemented in laminas/automatic-releases#1, which splits the application into multiple independent CLI commands.

@Ocramius Ocramius closed this Aug 2, 2020
@Ocramius Ocramius deleted the split-index branch August 2, 2020 00:20
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.

Extract further closures contained in public/index.php into independent units
2 participants