Skip to content
This repository has been archived by the owner on Jan 14, 2021. It is now read-only.

Migration to JSON::Serializable #211

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Migration to JSON::Serializable #211

wants to merge 4 commits into from

Conversation

MineBartekSA
Copy link

I've migrated all structs to the new JSON::Serializable in Crystal 0.35.0

Copy link
Contributor

@lucasintel lucasintel left a comment

Choose a reason for hiding this comment

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

thank you ❤️

Comment on lines 36 to 40
include JSON::Serializable
property icon : String?
property id : Snowflake
property members : Array(TeamMember)
property owner_user_id : Snowflake
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like the identation is a bit off here

Comment on lines 44 to 48
include JSON::Serializable
property membership_state : TeamMembershipState
property permissions : Array(String)
property team_id : Snowflake
property user : User
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like the identation is a bit off here

Comment on lines 6 to 8
struct IdentifyPacket
include JSON::Serializable
def initialize(server_id, user_id, session_id, token)
Copy link
Contributor

Choose a reason for hiding this comment

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

i think its a good practice to separate the include JSON::Serializable with one newline

e.g.

class Klass
  include JSON::Serializable

  property username : String?
end

@MineBartekSA
Copy link
Author

No problem! I'm glad I could help 😄
I was doing a lot of find and replace magic, so I missed some mistakes.
Will fix that tomorrow morning.

@z64
Copy link
Contributor

z64 commented Jun 14, 2020

@MineBartekSA You should able to run crystal tool format which will take care of any formatting issues for you.

@MineBartekSA
Copy link
Author

Oh, right, I totally forgot that this tool exists. Thanks for the tip

@MineBartekSA
Copy link
Author

Done, but still needed to add some new lines by hand

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants