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

Add tuple generator #71

Merged
merged 21 commits into from
May 28, 2024
Merged

Add tuple generator #71

merged 21 commits into from
May 28, 2024

Conversation

mtk3d
Copy link
Contributor

@mtk3d mtk3d commented Feb 19, 2023

Hello 😄
I prepared my proposal of Tuples with generics, as discussed in here #38
It's just proposal, still in progress, there is no tests yet.

I started by just coding this classes manually, but making them manually without mistakes is hard, and laborious. So following vavr's approach, I built a template with command that generates the code of the Tuples.
I'm not 100% contented by this solution, but that's the fastest and lightest solution that came to my mind.

I'm also wondering about using library like https://github.com/nette/php-generator. I could install it as a dev dependency and use it to generating the code in object oriented way, and it could be more readable maybe 🤔

@akondas What do you think about that?

@mtk3d mtk3d force-pushed the tuple-generics-proposal branch 3 times, most recently from 79d5831 to 86d358d Compare February 21, 2023 17:59
@mtk3d
Copy link
Contributor Author

mtk3d commented Feb 21, 2023

I've replaced it with better implementation using nette/php-generator. It works ways better in my opinion.
There is a test that is testing first 3 classes of Tuples by comparing strings, but I wrote them only in development purpose. In final implementation that doesn't make sense to keep them. Better will be to test only their behaviors and php-cs-fixer will check the code style.
It's still in progress and require a lot of refactoring.

@mtk3d mtk3d force-pushed the tuple-generics-proposal branch 3 times, most recently from dc1e0ec to 57a63fe Compare February 22, 2023 00:58
@mtk3d mtk3d changed the title Add tuple template and generator Add tuple generator Feb 22, 2023
@mtk3d mtk3d force-pushed the tuple-generics-proposal branch 4 times, most recently from 05a9088 to b33c329 Compare February 22, 2023 18:33
@mtk3d
Copy link
Contributor Author

mtk3d commented Feb 22, 2023

Todo:

  • Make some maintenance on master branch, do decrease size of PR Update dev dependencies #77
  • More test coverage for TupleN classes
  • Generator command CI checker, to avoid manual changes in generated classes

@mtk3d mtk3d force-pushed the tuple-generics-proposal branch 4 times, most recently from f3fa3fe to 0bffdb4 Compare February 22, 2023 22:21
@mtk3d
Copy link
Contributor Author

mtk3d commented Feb 22, 2023

Zrzut ekranu 2023-02-23 o 00 36 37

Ok, I belive I'm done. ~1300 lines of the PR is generated automatically, but it's still quite big, sorry for that 😄

I've added validation command to composer tests which is validating if existing Tuples has been generated by latest version of generator.
I covered Tuples in 100% in tests what I believe is quite important for generated classes.

@mtk3d mtk3d marked this pull request as ready for review February 22, 2023 23:31
@akondas
Copy link
Member

akondas commented Sep 19, 2023

looks cool, are you happy to rebase and wait for php 8.2 upgarde?

@mtk3d mtk3d force-pushed the tuple-generics-proposal branch from 85518d2 to 97824b6 Compare September 24, 2023 11:25
@mtk3d mtk3d force-pushed the tuple-generics-proposal branch from c67c442 to 33d4e4f Compare September 24, 2023 13:32
@mtk3d
Copy link
Contributor Author

mtk3d commented Sep 24, 2023

looks cool, are you happy to rebase and wait for php 8.2 upgarde?

Yes, thank you! 😄
I've refactored generate command a little bit. Right now it's a single command to validate and generate and it shows diff if validation fails:
Zrzut ekranu 2023-09-24 o 15 48 06

@@ -6,3 +6,5 @@ clover.xml
.idea
composer.lock
/.phpunit.cache/
.tuple/*
!.tuple/.gitkeep
Copy link
Member

Choose a reason for hiding this comment

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

I'm not against it, but couldn't we use temp dir?

@akondas
Copy link
Member

akondas commented Jan 8, 2024

Hey @mtk3d, Is there anything left to do here, because I would like to give it a chance, it's a big PR and I think it would be a pity to leave it open :)

@akondas
Copy link
Member

akondas commented May 26, 2024

Hi @mtk3d, ping again, If you don't have time right now, I can fix the changes after rebase and do a merge.

@akondas
Copy link
Member

akondas commented May 28, 2024

Closes #38

@akondas akondas merged commit 75528b1 into munusphp:master May 28, 2024
4 checks passed
@akondas
Copy link
Member

akondas commented May 28, 2024

Thanks @mtk3d 🍻

@mtk3d
Copy link
Contributor Author

mtk3d commented Jun 19, 2024

Hey @akondas sorry for not responding
I had an apartment renovation and it totally consumed all my free time and energy
Thanks for merge 🍻

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.

2 participants