Skip to content

Fix/message create event#13

Open
dixtel wants to merge 6 commits into
grottohub:mainfrom
dixtel:fix/message_create_event
Open

Fix/message create event#13
dixtel wants to merge 6 commits into
grottohub:mainfrom
dixtel:fix/message_create_event

Conversation

@dixtel
Copy link
Copy Markdown
Contributor

@dixtel dixtel commented Apr 6, 2024

No description provided.

@dixtel
Copy link
Copy Markdown
Contributor Author

dixtel commented Apr 6, 2024

@grottohub I see that you merged a lot of things to main and it will not be so easy to merge with my changes.
In short my changes are about:

  • Message type and dependency
  • codegen for discord module types (gleam run -m glyph/internal/codegen to generate all decoders)

@grottohub
Copy link
Copy Markdown
Owner

great! ill take a look through this today, but as its a bit sizable and ill need to think about merge strategy, might not have full feedback until tomorrow

Copy link
Copy Markdown
Owner

@grottohub grottohub left a comment

Choose a reason for hiding this comment

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

just a few questions / tweaks. overall awesome work! personally im fine with merging this in once the .toml file conflicts get resolved - its ok if main is a bit unstable while we make such a big change in how we're doing decoders, then ill go in and make any necessary tweaks to the types that i need to.

Comment thread gleam.toml Outdated
logging = "~> 1.0"
prng = "~> 3.0"
carpenter = "~> 0.2"
glyph_codegen = "~> 0.3"
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

this should go in [dev-dependencies]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yep, my bad

Comment thread gleam.toml Outdated

target = "erlang"
gleam = ">= 1.0.0"
gleam = ">= 0.32.0"
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

is there a reason we've lowered the minimum version?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just mistake

Comment on lines +91 to +92
debug(event)
debug(message)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

these should be removed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yep, my bad

@dixtel
Copy link
Copy Markdown
Contributor Author

dixtel commented Apr 7, 2024

@grottohub I will fix these issues today or tomorrow

@dixtel
Copy link
Copy Markdown
Contributor Author

dixtel commented Apr 8, 2024

@grottohub I fixed these conflicts. In this week I will not have time for gleam/glyph 😞

I'm really want to finish glyph_codegen because right now the conversion between field type to dynamic.* is hard-coded (e.g. Option(String) => dynamic.optional_field(dynamic.string)) and I don't like it. This will cause errors for field types not supported in glyph_codegen (This can happen when merging this MR)

I want to create generic codegen for json that accept any field type and share with the gleam community.

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