Implemented nullable association using empty by stekycz · Pull Request #5 · Tharos/Schematic

stekycz · 2017-02-27T22:19:44Z

Refs #4

Tharos · 2017-02-28T17:31:03Z

Very nice solution, thank you!

I'm going to push doc update and after that let's merge this. :)

Tharos · 2017-03-01T19:48:53Z

Dovolil jsem si to rebasnout s ohledem na #6.

Autorství úprav jsem samozřejmě zachoval. :)

stekycz · 2017-03-01T20:27:08Z

Stihl jsem udělat už předtím, ale dobře :P

meridius · 2017-03-02T08:18:28Z

src/Schematic/Entry.php

-		if ($data === NULL) {
+		if (!$association[self::INDEX_MULTIPLICITY] && (
+		    $data === NULL
+            || ($association[self::INDEX_NULLABLE] && empty($data))


Je to drobnost, ale tady používáš space, místo TAB.

A koukám, že jinde občas taky.

meridius · 2017-03-02T12:17:38Z

src/Schematic/Entry.php

 	const INDEX_ENTRYCLASS = 0;
 	const INDEX_MULTIPLICITY = 1;
 	const INDEX_EMBEDDING = 2;
+	const INDEX_NULLABLE = 3;


Konstanta INDEX_NULLABLE by měla být v doc pro self::$parsedAssociations.

meridius · 2017-03-02T22:05:00Z

tests/SchematicTests/EntryTest.phpt

+		$lastOrderItem = end($orderItems);

 		Assert::type(CustomEntries::class, $firstOrderItem->tags);
+		Assert::type(CustomEntries::class, $lastOrderItem->tags);


Tohle se mi nezdá. Přece pokud mám $lastOrderItem->tags definované jako nullable multiple a plním ho "empty" hodnotami, tak by mělo vracet NULL.

Nevím, jaký má smysl nullable ne-multiple fileld, ale pokud nepřekáží, tak by měl pro empty hodnoty asi vracet NULL. Pokud uživatele budou empty hodnoty zajímat, tak by takový field neměl označovat jako nullable.

Pro přehlednost tabulka, jak si myslím, že by to mělo být:

normal nullable multiple nullable multiple

text text text TypeError TypeError

number number number TypeError TypeError

data array Entry Entry Entries Entries

empty(value) empty(value) null TypeError null

null null null TypeError null

empty array empty array null Entries null

0 0 null TypeError null

Hmm, na tom něco bude. Nechal jsem se asi příliš unést tím, že nutím svůj přístup, že každá kolekce by měla být vždy kolekce a nikdy null, ale to by si měl člověk zvolit podle toho, co tam chce mít. Upravím.

meridius · 2017-03-02T22:07:46Z

tests/SchematicTests/EntryTest.phpt



-	public function testEmbeddedEntries()
+	public function provideDataNullableCollection()


Vzhledem k tomu, že to vrací stejné "empty" hodnoty jako provideDataNullableRelation(), tak bych oboje sjednotil pod něco jako provideDataNullableValues() nebo tak.

meridius · 2017-03-02T22:33:51Z

src/Schematic/Entry.php

+				self::INDEX_EMBEDDING => !empty($matches[3]) ?
+					($matches[3] === '.' ? $matches[2] . '_' : substr($matches[3], 1)) :
 					FALSE,
+				self::INDEX_NULLABLE => !empty($matches[1]),


Tady bych možná empty() nahradil výsledkem volání protected funkce isEmpty() (nebo tak nějak), aby si uživatel měl možnost nastavit, jaké hodnoty považuje za ty správně "empty". Dovedu si představit, že asi ne všem se trefíme do use-case.

meridius · 2017-03-02T22:41:53Z

src/Schematic/Entry.php

 			$this->readData($name);

-		if ($data === NULL) {
+		if (!$association[self::INDEX_MULTIPLICITY] && (


Banalita, ale nechtělo by takhle složitý if učesat třeba takhle?

if ( !$association[self::INDEX_MULTIPLICITY] && ( $data === NULL || ($association[self::INDEX_NULLABLE] && empty($data)) ) ) {

stekycz · 2017-03-03T11:28:12Z

Upraveno dle komentářů od @meridius a rozšířeny testy snad na všechny možné kombinace hodnot a nastavení properties a asociací. Jen mi tam trochu nesedí to, že jakmile je hodnota NULL, tak i "required" asociace může být null. Asi dává smysl, protože nullable přidává spíše "emptynest".

Tak jen přemýšlím o přidání !, aby nemohla být hodnota null vůbec nikdy. Odebrat tu podmínku na null by teď totiž byl BC break. Samozřejmě by tam musela být podmínka, že ! a ? nelze kombinovat :-)

Tharos · 2017-03-07T19:27:15Z

Ahoj vespolek, jenom chci dát vědět, že tenhle pull budu řešit ASAP… Mám nějaký zdravotní trable, ale jakmile mi bude líp, vše projdu. Snad už tento týden. Díky za trpělivost! :)

meridius · 2017-03-07T20:14:52Z

@Tharos Děkuju za update a držím palce, ať je ti brzo líp.

Tharos mentioned this pull request Mar 1, 2017

Nepovinný parametr #4

Open

stekycz force-pushed the nullable-association branch from 16ea2bc to 5f49ba6 Compare March 1, 2017 19:41

Tharos force-pushed the nullable-association branch 3 times, most recently from 3d20766 to 12a6252 Compare March 1, 2017 19:47

stekycz force-pushed the nullable-association branch from 12a6252 to 330005d Compare March 2, 2017 00:28

meridius reviewed Mar 2, 2017

View reviewed changes

stekycz force-pushed the nullable-association branch from 330005d to 5448ab7 Compare March 2, 2017 08:22

Implemented nullable association using empty

e2b8b1e

stekycz force-pushed the nullable-association branch from 5448ab7 to e2b8b1e Compare March 2, 2017 08:23

meridius reviewed Mar 2, 2017

View reviewed changes

meridius suggested changes Mar 2, 2017

View reviewed changes

stekycz added 6 commits March 3, 2017 11:15

Added doc comment for nullable index in parsed associations

8e5bea2

Used one generic data provider instead of two

634325b

Made nullable condition more readable

e3bda86

Allowed overriding function for empty value detection

815d33d

Updated entry tests to validate all property combinations

e1572c6

Fixed entry class to support nullable collections

d8da1bb

	normal	nullable	multiple	nullable multiple
text	text	text	TypeError	TypeError
number	number	number	TypeError	TypeError
data array	Entry	Entry	Entries	Entries
empty(value)	empty(value)	null	TypeError	null
null	null	null	TypeError	null
empty array	empty array	null	Entries	null
0	0	null	TypeError	null



		public function testEmbeddedEntries()
		public function provideDataNullableCollection()

Comments

Conversation

stekycz commented Feb 27, 2017

Uh oh!

Tharos commented Feb 28, 2017

Uh oh!

Tharos commented Mar 1, 2017

Uh oh!

stekycz commented Mar 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stekycz commented Mar 3, 2017

Uh oh!

Tharos commented Mar 7, 2017

Uh oh!

meridius commented Mar 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants