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

Changed and unified structure (BC breaks) #28

Merged
merged 1 commit into from
Jun 9, 2023

Conversation

MartinMystikJonas
Copy link
Contributor

Zkus to prosím zkouknout. JE tam hodně změn takže changelog je dost matoucí. Možná by bylo lepší zkouknout výslednou strukturu přímo.

Pokud to bude ok opravím a doplním ještě readme.

@MartinMystikJonas
Copy link
Contributor Author

Změny ve stručnosti:

  • všechny modely přesunuté do Models, některé z nihc přejemnované kvůli konfliktům názvů
  • namespace Requsts -> Endpoints
  • sjednocená struktura Endpointů a Requestů
  • Requesty mají generiky určující jakou vrací Response
  • Response mají generiku určující jaká vrací data
  • abstrakce obvyklých typů response StatusResponse, CollectionResponse, ItemResponse
  • odstraněné některé duplikace kódu
  • nějaké drobné úpravy a odstranění zbytečného kódu všude možně

@pionl
Copy link
Owner

pionl commented May 4, 2023

Můžeš prosím tě přidat UPGRADE.md a kapitolu 0.4.1 -> 1.0.0 ?

A vypsat tam cca postup jak to "rozumně upgradovat" ze staré verze na novou?

@MartinMystikJonas
Copy link
Contributor Author

Doplněno a upravil sme i README. Ještě to bude asi chtít trochu učesat než to mergneme, ale už bys to mohl zkouknout jestli je to za tebe ok takhle.

@pionl
Copy link
Owner

pionl commented May 4, 2023

Určitě podívám, mám to tady otevřené a postupně na to koukám :) Dnes večer to budeš mít

@pionl
Copy link
Owner

pionl commented May 4, 2023

@stanislav-janu chceš se na změny podívat?

Copy link
Owner

@pionl pionl left a comment

Choose a reason for hiding this comment

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

Skvělá práce, upgrade guide je jasný a dobře napsaný!

Někde ještě chybí typehinty, na to je myslím i rector rule.

Přidal bych všude typehint (min object / stdClass / self / void apod).

Díky!

src/Endpoints/AbstractCollectionResponse.php Outdated Show resolved Hide resolved
src/Endpoints/AbstractDataResponse.php Show resolved Hide resolved
src/Endpoints/AbstractDataResponse.php Show resolved Hide resolved
src/Models/Holder/ContactLists.php Outdated Show resolved Hide resolved
src/Models/Holder/ContactLists.php Outdated Show resolved Hide resolved
src/Models/Holder/CustomFieldValues.php Outdated Show resolved Hide resolved
src/Models/Holder/CustomFieldValues.php Outdated Show resolved Hide resolved
@MartinMystikJonas
Copy link
Contributor Author

Typehintu tam bude chybet vic. Planuju pozdeji prejit na PHPStan level 8 a vsude je doplnit.

@stanislav-janu
Copy link
Contributor

@stanislav-janu chceš se na změny podívat?

Krom typehintů a pár věcí, co jsem poslal jako line comment tam z mého pohledu nevidím žádný zádrhel. Chce to ještě vyzkoušet v reálu. Koukám jen, že nekteré testy jsou skipnuty. Víme proč?

{
return array_filter($this->toArray(), static function ($val) {
return array_filter($data, static function ($val) {
Copy link
Contributor

Choose a reason for hiding this comment

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

zde si myslím, že je static zbytečný

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je to mikrooptimalizace, ale ušetří binding $this.

@stanislav-janu
Copy link
Contributor

stanislav-janu commented May 5, 2023

Nechceme používat konstanty jako CamelCase? Tak jako na to přešlo Nette? Uppercase je docela přežitek. Ale je to jen kosmetika.
A nepreferuji mezeru za vykřičníkem. Preferuji zápis if (!$param). Taky jen kosmetika k zamyšlení.

@MartinMystikJonas
Copy link
Contributor Author

Skipnute jsou testy, ktere by realne zkousely sahat na live API. Pri testovani je mozne skipnuti zakomentovat a vyzkouset jak to funguje.

Ohledne nazvu konstant je mi to v zasade jedno. Pomud prederujete CamelCase muzu predelat.

Osttani kosmeticke veci veci jsou defaultni ECS - nevim jestli ma cenu ho prenastavovat.

@MartinMystikJonas
Copy link
Contributor Author

Mělo by to but ready na merge. Změnu conding standardu kdyžtak můžem udělat později.

@MartinMystikJonas
Copy link
Contributor Author

Je ode mě ještě něco potřeba aby se to mergnulo?

@MartinMystikJonas
Copy link
Contributor Author

@pionl Muzes tohle prosim mergnout? Mam ready navazujici PR, který přidává typy. A pak bych chtěl dodělat ty endpointy co mi chybí.

@MartinMystikJonas MartinMystikJonas mentioned this pull request May 17, 2023
@pionl
Copy link
Owner

pionl commented Jun 9, 2023

všem se omlouvám, měl jsem ted celkem mazec.

Takto je to super. Za mě constanty UpperCase, kde je možné enum (když už se to toho takto šahá :)

A díky moc @MartinMystikJonas za skvělý příspěvek do projektu (a věřím že i oživení).

@stanislav-janu díky za review!

@pionl pionl merged commit f40cd05 into pionl:master Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants