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

Added missing segments API calls (v11) #178

Merged
merged 51 commits into from
Dec 8, 2023

Conversation

rcerljenko
Copy link
Contributor

Hi,

In addition to my previous work, this one adds a new "Segments" API calls for Create, List and Delete actions.

@norkunas
Copy link
Owner

As discussed in #172 we should start with request payload objects and static analysis instead of data resolvers in a forward compatible way and then deprecate the old apis :)

@rcerljenko
Copy link
Contributor Author

How do we validate payload objects? Via phpdoc ?

@norkunas
Copy link
Owner

Yes, with SA tools, so no overhead on runtime perf :)

@rcerljenko
Copy link
Contributor Author

Ok I'll try

@norkunas
Copy link
Owner

Instead of direct arrays add an payload object so users instantiate it and get autocomplete for available methods :)

@rcerljenko
Copy link
Contributor Author

@norkunas check it out now if that's it

@rcerljenko
Copy link
Contributor Author

aha I see... you mean with some sort of DTO?

@norkunas
Copy link
Owner

Yes ;)

src/Segments.php Outdated Show resolved Hide resolved
src/Segments.php Outdated Show resolved Hide resolved
src/Segments.php Outdated Show resolved Hide resolved
@rcerljenko
Copy link
Contributor Author

@norkunas how about now? i'll just fix the fixer erros if that's ok

Copy link
Owner

@norkunas norkunas left a comment

Choose a reason for hiding this comment

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

Looks better 😉

src/Dto/Segments/CreateSegment.php Outdated Show resolved Hide resolved
src/Dto/Segments/CreateSegment.php Outdated Show resolved Hide resolved
src/Dto/Segments/CreateSegment.php Outdated Show resolved Hide resolved
src/Dto/Segments/ListSegments.php Outdated Show resolved Hide resolved
src/Dto/Segments/ListSegments.php Outdated Show resolved Hide resolved
src/Dto/Segments/ListSegments.php Outdated Show resolved Hide resolved
src/Dto/Segments/ListSegments.php Outdated Show resolved Hide resolved
src/Dto/Segments/ListSegments.php Outdated Show resolved Hide resolved
src/Dto/Segments/ListSegments.php Outdated Show resolved Hide resolved
@rcerljenko rcerljenko requested a review from norkunas November 30, 2023 10:31
@rcerljenko rcerljenko requested a review from norkunas December 5, 2023 12:29
@norkunas
Copy link
Owner

norkunas commented Dec 5, 2023

good 💪 now tests, after this PR will try to spend time on this also :)

tests/SegmentsTest.php Outdated Show resolved Hide resolved
@rcerljenko rcerljenko requested a review from norkunas December 8, 2023 11:40
@rcerljenko rcerljenko requested a review from norkunas December 8, 2023 11:43
@rcerljenko rcerljenko requested a review from norkunas December 8, 2023 11:58
norkunas
norkunas previously approved these changes Dec 8, 2023
Copy link
Owner

@norkunas norkunas left a comment

Choose a reason for hiding this comment

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

Could you squash all commits? :)

@rcerljenko
Copy link
Contributor Author

rcerljenko commented Dec 8, 2023

Could you squash all commits? :)

Can't you do that on merge?

Also, I still have to write test for "Create segment" method.

@rcerljenko
Copy link
Contributor Author

rcerljenko commented Dec 8, 2023

@norkunas norkunas merged commit af80fa0 into norkunas:master Dec 8, 2023
11 of 13 checks passed
@norkunas
Copy link
Owner

norkunas commented Dec 8, 2023

Thank you :) now will need to add remaining apis and deprecate old ones for a new release

@rcerljenko rcerljenko deleted the feature/segments branch December 8, 2023 12:50
@rcerljenko
Copy link
Contributor Author

Thank you :) now will need to add remaining apis and deprecate old ones for a new release

Yes. Bit by bit... :)
But at least we now have a "modus operandi"

@norkunas
Copy link
Owner

norkunas commented Dec 8, 2023

true :)

@rcerljenko
Copy link
Contributor Author

will you trigger a new release soon?

@norkunas
Copy link
Owner

norkunas commented Dec 8, 2023

Wont do partial releases for now

@norkunas
Copy link
Owner

norkunas commented Dec 8, 2023

If you need you can always use dev branch in composer json :)

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