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

Fix inconsistency issues #37

Merged
merged 2 commits into from
Aug 4, 2020
Merged

Fix inconsistency issues #37

merged 2 commits into from
Aug 4, 2020

Conversation

jkmassel
Copy link
Contributor

  • Makes the APNSConfiguration.getEndpoint method have consistent casing
  • Adds a toJSON method on APNSPayload that provides consistency with other encodable objects and consistency when encoding.

@jkmassel jkmassel added the bug Something isn't working label Jul 31, 2020
@jkmassel jkmassel requested a review from mokagio July 31, 2020 22:28
@jkmassel jkmassel self-assigned this Jul 31, 2020
@codeclimate
Copy link

codeclimate bot commented Jul 31, 2020

Code Climate has analyzed commit 277fbd3 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 83.2% (0.0% change).

View more on Code Climate.

@@ -121,14 +121,13 @@ function new_credentials(): APNSCredentials {
return new APNSCredentials( $this->random_string( 10 ), $this->random_string( 10 ), '' );
}

function encode( $object ) {
function to_stdclass( $object ): object {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've renamed this method to make things clearer at the call site.

$object = $this->encode_to_array( $push );
unset( $object['aps'] );
$this->assertEquals( $custom_data, $object );
$object = $this->to_stdclass( $push );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

encode_to_array was only used in one place, so I got rid of it and am using objects instead for consistency.

Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

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

Looks consistent.

I looked up the docs for JsonSerializable,

Objects implementing JsonSerializable can customize their JSON representation when encoded with json_encode().

From here, it seems that the preferred method to encode to JSON from the library is now to call toJSON(). Am I correct? What's the advantage over a the standard (is it standard?) json_encode() approach? One good thing that stands out to me is that with toJSON() we get better looking -> chaining.

@jkmassel
Copy link
Contributor Author

jkmassel commented Aug 4, 2020

Yeah, I don't mind re-adding JsonSerializable conformance later, but the inconsistency wasn't great – I also like the chainability, but I think it'd make sense to go around and add the serialization to everything all at once

@jkmassel jkmassel merged commit 5dbb3a1 into main Aug 4, 2020
@jkmassel jkmassel deleted the fix/inconsistency-issues branch August 4, 2020 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants