Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add command settings:publish #80

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

datamweb
Copy link
Contributor

No description provided.

@datamweb
Copy link
Contributor Author

No idea 😇

Screenshot 2023-05-20 114417

phpstan.neon.dist Outdated Show resolved Hide resolved
@datamweb datamweb force-pushed the add-command-publish-config branch from d52ebc4 to 0b113d9 Compare May 20, 2023 08:43
@paulbalandan paulbalandan changed the title fate: add command settings:publish feat: add command settings:publish May 20, 2023
src/Commands/SettingsPublisher.php Outdated Show resolved Hide resolved
src/Commands/SettingsPublisher.php Outdated Show resolved Hide resolved
Comment on lines +46 to +49

if (is_file($filepath)) {
copy($filepath, APPPATH . 'Config/Settings.php.bak');
}
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need this? Since there's no unlinking in the tearDown, this operation would unnecessarily pollute the next test and the filesystem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paulbalandan In fact, testPublishConfigFileWithForce() depends on this. I wanted to write in the following way, but apparently the notification will be removed in phpunit v10.1:

/**
* @depends testPublishConfigFile
*/
public function testPublishConfigFileWithForce(): void
{
// ...
}

phpunit v10.1:

#[Depends('testPublishConfigFile')]
public function testPublishConfigFileWithForce(): void
{
// ...
}

Copy link
Member

Choose a reason for hiding this comment

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

I think instead of depending on the other test, in the 2nd test you should run the command twice. First without the -f flag asserting the file exists. Then another command run with the -f flag.

@kenjis kenjis added the enhancement New feature or request label May 22, 2023
- APP_NAMESPACE
- CI_DEBUG
- ENVIRONMENT
parameters:
Copy link
Member

Choose a reason for hiding this comment

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

What was changed here? Tabs to spaces?

Copy link
Member

Choose a reason for hiding this comment

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

@datamweb Please keep this file as close to the DevKit template as possible so updates are easier.


try {
$publisher->addPath('Config/Settings.php')
->merge($this->overwrites);
Copy link
Member

Choose a reason for hiding this comment

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

If $this->overwrites is only needed in this particular place, then you don't need the property. You can simply use array_key_exists('f', $params) here or use an intermediate variable.

Comment on lines +74 to +75
$publisher->addPath('Config/Settings.php')
->merge($this->overwrites);
Copy link
Member

Choose a reason for hiding this comment

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

Can you align ->addPath to the ->merge below?

Suggested change
$publisher->addPath('Config/Settings.php')
->merge($this->overwrites);
$publisher
->addPath('Config/Settings.php')
->merge($this->overwrites);

Comment on lines +46 to +49

if (is_file($filepath)) {
copy($filepath, APPPATH . 'Config/Settings.php.bak');
}
Copy link
Member

Choose a reason for hiding this comment

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

I think instead of depending on the other test, in the 2nd test you should run the command twice. First without the -f flag asserting the file exists. Then another command run with the -f flag.

* @depends testPublishConfigFile
*/
public function testPublishConfigFileWithForce(): void
{
Copy link
Member

Choose a reason for hiding this comment

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

Remove extra whitespaces after { and before }.

Comment on lines +58 to +66
$filepath = APPPATH . 'Config/Settings.php';

helper('filesystem');
write_file($filepath, 'fake text.');
$contents = $this->getFileContents($filepath);

$this->assertFileExists($filepath);
$this->assertStringContainsString('fake text.', $contents);

Copy link
Member

Choose a reason for hiding this comment

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

This part basically tests write_file, not the command.

Comment on lines +72 to +75
clearstatcache(true, $expectedConfigFile);
if (is_file($expectedConfigFile)) {
unlink($expectedConfigFile);
}
Copy link
Member

Choose a reason for hiding this comment

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

cleanup should be done in teardown.

Copy link
Member

Choose a reason for hiding this comment

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

If the cleanup is only for the test method, I think it is okay to put it in the test method.
tearDown() is called after each test method.

Copy link
Member

Choose a reason for hiding this comment

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

The advantage of tearDown() is that it will always run regardless of test failures. Especially if there are multiple tests that might leave this file lingering I like the idea of an idempotent cleanup after every test.

@datamweb datamweb marked this pull request as draft May 23, 2023 08:37
@MGatner
Copy link
Member

MGatner commented Jul 4, 2023

@datamweb This looks good, and close! Can you get the recommendations in place and we can get this in for next release?

@datamweb
Copy link
Contributor Author

datamweb commented Jul 5, 2023

Can you get the recommendations in place and we can get this in for next release?

@MGatner I'm currently traveling, I'll try to get back to the reviews as soon as I get back to my PC.

@lonnieezell
Copy link
Member

@datamweb any chance you'd be able to wrap this up? I've got another command about ready to merge and it would be great to release them together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants