-
Notifications
You must be signed in to change notification settings - Fork 0
[WIP] Initial Component Structure #1
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
base: develop
Are you sure you want to change the base?
Changes from all commits
2f62a5e
6ccf869
0cb16b7
caf2a04
306f462
80f0b09
273e0d5
76ee32b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
/vendor/ | ||
composer.lock | ||
phpunit.xml | ||
/coverage/ | ||
.php_cs.cache |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
<?php | ||
|
||
return PhpCsFixer\Config::create() | ||
->setRiskyAllowed(true) | ||
->setRules(array( | ||
'@PSR2' => true, | ||
'psr4' => true, | ||
'ordered_class_elements' => true, | ||
'ordered_imports' => true, | ||
'phpdoc_add_missing_param_annotation' => true, | ||
'phpdoc_order' => true, | ||
)) | ||
->setFinder( | ||
PhpCsFixer\Finder::create() | ||
->exclude('tests/Fixtures') | ||
->exclude('vendor') | ||
->in(__DIR__) | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
{ | ||
"name": "phpol/string", | ||
"description": "PHP Object Library String Component", | ||
"type": "library", | ||
"license": "MIT", | ||
"minimum-stability": "stable", | ||
"require": {}, | ||
"autoload": { | ||
"psr-4": { | ||
"OL\\String\\": "src/" | ||
} | ||
}, | ||
"require-dev": { | ||
"phpunit/phpunit": "^6.0", | ||
"friendsofphp/php-cs-fixer": "^2.0" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Likewise, cs-fixer should be something we all have installed in our dev environments (something like phpcbf), not a part of the project, IMO. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we fix a PHP version via composer? |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<phpunit backupGlobals="false" | ||
backupStaticAttributes="false" | ||
colors="true" | ||
convertErrorsToExceptions="true" | ||
convertNoticesToExceptions="true" | ||
convertWarningsToExceptions="true" | ||
processIsolation="false" | ||
stopOnFailure="false"> | ||
<testsuites> | ||
<testsuite name="String Component Test Suite"> | ||
<directory suffix="Test.php">./tests</directory> | ||
</testsuite> | ||
</testsuites> | ||
<filter> | ||
<whitelist processUncoveredFilesFromWhitelist="true"> | ||
<directory suffix=".php">./src</directory> | ||
</whitelist> | ||
</filter> | ||
<logging> | ||
<log type="coverage-clover" target="coverage/coverage.xml"/> | ||
<log type="coverage-html" target="coverage/html"/> | ||
</logging> | ||
</phpunit> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
<?php | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Maybe if we intend to implement both immutable (which in this context should be akin to constant) strings and mutable strings, it would make sense to have them both implement a contract. Maybe I don't care if my string is mutable. I'm not sure if it makes sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should support both, as suggested here, but reinforce the immutable state by suffixing the class. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the goal of having immutable strings? What do we expect to accomplish with it? I understand keeping track of object state is important, and immutability helps in that aspect. But if an object is a primitive type wrapper, is its state important? As a side note, primitive types are always passed by copy in PHP, where objects are always passed as references. In this aspect, immutable objects replicate best a primitive type, as any modification will not propagate everywhere. The performance loss greatly concerns me, though. Benchmarks would be greatly appreciated here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @naroga if we're using as objects, we cannot copy the variable without clone. Remember they are objects so if we do a toUpperCase() on the variable, it will have undesired effects if not immutable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the record, this discussion has been settled in issue #4. |
||
|
||
namespace OL\String\Contracts; | ||
|
||
/** | ||
* Interface Str. | ||
* | ||
* This interface should be the core interface for all string related classes. | ||
* Other classes within this component should (when possible) extend this one. | ||
*/ | ||
interface Str | ||
{ | ||
/** | ||
* Str constructor. | ||
* | ||
* The constructor will enforce string type, since it can also receive | ||
* another Str object (self) as it can be casted to string. | ||
* | ||
* @param string|Str $payload The actual string | ||
*/ | ||
public function __construct(string $payload); | ||
|
||
/** | ||
* Str objects should should be able to be cast as string. | ||
* | ||
* @return string The actual string | ||
*/ | ||
public function __toString(); | ||
|
||
/** | ||
* Public method to retrieve the raw string contents of an object. | ||
* | ||
* @return string | ||
*/ | ||
public function contents(); | ||
|
||
/** | ||
* Replace one or more entries from the given object with a given value | ||
* | ||
* @param array|string $search What to search for | ||
* @param string $replace Replace with? | ||
* | ||
* @return Str | ||
*/ | ||
public function replace($search, string $replace); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
<?php | ||
|
||
namespace OL\String; | ||
|
||
use OL\String\Contracts\Str as StrContract; | ||
|
||
/** | ||
* Class Str. | ||
* | ||
* Basic manipulation of strings. | ||
*/ | ||
class Str implements StrContract | ||
{ | ||
/** | ||
* @var string Raw string content | ||
*/ | ||
protected $payload; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather have this be named There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whatever |
||
|
||
/** | ||
* Str constructor. | ||
* | ||
* The constructor will enforce string type, since it can also receive | ||
* another Str object (self) as it can be casted to string. | ||
* | ||
* @param string|StrContract $payload The actual string | ||
*/ | ||
public function __construct(string $payload) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we have static factories, so we can call e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or default for all, |
||
{ | ||
// casts the payload as string and them assign on class scope | ||
$this->payload = (string) $payload; | ||
} | ||
|
||
/** | ||
* Str objects should should be able to be cast as string. | ||
* | ||
* @return string The actual string | ||
*/ | ||
public function __toString() | ||
{ | ||
return $this->contents(); | ||
} | ||
|
||
/** | ||
* Public method to retrieve the raw string contents of an object. | ||
* | ||
* @return string | ||
*/ | ||
public function contents() | ||
{ | ||
return $this->payload; | ||
} | ||
|
||
/** | ||
* Replace one or more entries from the given object with a given value | ||
* | ||
* @param array|string $search What to search for | ||
* @param string $replace Replace with? | ||
* | ||
* @return StrContract Object instance with the replaced text. | ||
*/ | ||
public function replace($search, string $replace) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I strongly disagree with using a factory to run a replace here. This wrapper should be as performant as possible, and it should do this by avoiding instantiation of redundant objects. Adding a big footprint just to OOPize things will greatly hinder this library's adoption. With that in mind, mutable objects should be preferred, as they wouldn't require new objects to be instantiated on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think, at least for the "primitive components" (string, number and maybe array), we should use immutable objects. Immutability achieves a defensive way of programming by preventing objects to change their state, which is a preference for some developers. Also, it's the same implementation of the native PHP functions, making the transition from the existing code to PHP-OL more "natural". I also think this should be discussed in project-level. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @marcossffilho If our goal is to port the primitive types to a OO fashion with as little footprint as possible, then immutability is a moot point. The primitive types are mutable themselves, and changing this behavior by default breaks the "porting" aspect of the library. PHP strings are mutable, but our strings won't be? Also, immutability costs more memory and CPU to perform operations, as new instances have to be created.
In the previous snippet, if
Immutability brings little to no value to primitive types, as I presented in my other comment. It brings value to objects that need to be state-tracked. Primitives don't need to have their states tracked. However, if Note that even if we opt to make the object immutable, the variable itself is still mutable, and in the second snippet, I actively changed I will repeat: when object state needs to be tracked (and they need to be tracked most of the time), immutability helps. When object state doesn't need to be tracked (and primitives don't need their state tracked), immutability is moot. A wrapper for a primitive should also not need to rely in its state, only in its content value. Other data types ( I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also noting that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not so sure with "PHP strings are mutable" because they are primitives. Primitives have no state, they're just a value. When you change a string variable, you don't mutate the value, you just replace it with a new one. But when I said that immutability is the same implementation of native PHP functions, I refer to the fact that common string functions like I understand the point about performance. And also agree if we're going to have both "Str" and "StrImmutable" (I don't like the Another point that I want to show is template strings: <?php
$tpl = new Str('[Person] love PHP-OL!');
foreach (['naroga', 'marcossffilho', 'hernandev', 'everyone'] as $person) {
echo $tpl->replace('[Person]', $person);
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An example on string mutability:
I have created a string, and made modifications to a subset of the string. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In an analogous fashion, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want to add this use-case for discussion (if it's relevant): <?php
$str = new Str('a string');
$start = $str->slice(0, 3);
$end = $str->slice(-3); |
||
{ | ||
return $this->factory(str_replace($search, $replace, $this)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very important! Please note that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oriental (japanese, chinese, korean, arabic, etc) characters do not work with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or, we can also require There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @marcossffilho I don't need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't mind the verification, but IMHO |
||
} | ||
|
||
/** | ||
* Factories a new StrContract object. | ||
* | ||
* @param string $str Instance to be factory from | ||
* | ||
* @return StrContract | ||
*/ | ||
protected function factory(string $str) | ||
{ | ||
return new self($str); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Return new static($str); is better, IMO? You can create instances regardless the subtype that is calling Str::factory method. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
<?php | ||
|
||
namespace OL\String; | ||
|
||
use OL\String\Contracts\Str as StrContract; | ||
use PHPUnit\Framework\TestCase; | ||
|
||
class StrTest extends TestCase | ||
{ | ||
/** | ||
* This test should instantiate with success from a string | ||
*/ | ||
public function test_instantiable_with_string() | ||
{ | ||
// create a new instance | ||
$str = new Str('foo value'); | ||
|
||
// make sure it is implementing the main contract | ||
$this->assertInstanceOf(StrContract::class, $str); | ||
} | ||
|
||
/** | ||
* This test should determine if an object of Str can be cast as a string. | ||
*/ | ||
public function test_object_can_be_cast_as_string() | ||
{ | ||
// create a new instance | ||
$str = new Str('foo bar baz'); | ||
|
||
// assert the cast value of the object is equals the initially given string. | ||
$this->assertEquals('foo bar baz', (string) $str); | ||
} | ||
|
||
/** | ||
* This test should prove an Str object can be created from another | ||
* Str object. | ||
*/ | ||
public function test_instantiable_with_str_object() | ||
{ | ||
// create an initial instance | ||
$initialStr = new Str('foo bar'); | ||
|
||
// create an instance passing not directly a string but an StrContract object. | ||
$str = new Str($initialStr); | ||
|
||
// assets contract implementation. | ||
$this->assertInstanceOf(StrContract::class, $str); | ||
// assets the contents are correctly passed between instances. | ||
$this->assertEquals('foo bar', $str->contents()); | ||
} | ||
|
||
/** | ||
* Test replacement of parts of text, using a single string | ||
*/ | ||
public function test_replace_single_string() | ||
{ | ||
$str = new Str('foo bar'); | ||
|
||
$replaced = $str->replace('bar', 'baz'); | ||
|
||
$this->assertEquals('foo baz', $replaced); | ||
} | ||
|
||
/** | ||
* Test replacement of parts of text, using a multiple search strings | ||
*/ | ||
public function test_replace_multiple_string() | ||
{ | ||
$str = new Str('foo bar baz'); | ||
|
||
$replaced = $str->replace(['bar', 'baz'], 'foo'); | ||
|
||
$this->assertEquals('foo foo foo', $replaced); | ||
} | ||
|
||
/** | ||
* Test replacement of parts of text, using a multiple search strings and objects | ||
*/ | ||
public function test_replace_multiple_string_with_objects() | ||
{ | ||
$str = new Str('foo bar baz'); | ||
|
||
$replaced = $str->replace([new Str('bar'), 'baz'], 'foo'); | ||
|
||
$this->assertEquals('foo foo foo', $replaced); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be versioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.php_cs.dist should, is a simple way to enforce code style, this one don't need to versioned indeed