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 Guild Scheduled Event Support #1032

Merged
merged 52 commits into from Feb 27, 2022
Merged

Conversation

42atomys
Copy link
Contributor

Discord just released a new feature: Guild Scheduled Event

Today I will start to add all endpoints listed in the API Reference (https://discord.com/developers/docs/resources/guild-scheduled-event#guild-scheduled-event)

Following this issue : #1031

Copy link
Contributor

@QPixel QPixel left a comment

Choose a reason for hiding this comment

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

You'll also have to update your entire branch with the current state of master. @FedorLap2006 any additional things I didn't catch?

scheduled_events.go Outdated Show resolved Hide resolved
scheduled_events.go Outdated Show resolved Hide resolved
scheduled_events.go Outdated Show resolved Hide resolved
scheduled_events.go Outdated Show resolved Hide resolved
scheduled_events.go Outdated Show resolved Hide resolved
structs.go Outdated Show resolved Hide resolved
structs.go Show resolved Hide resolved
scheduled_events.go Outdated Show resolved Hide resolved
scheduled_events.go Outdated Show resolved Hide resolved
scheduled_events.go Outdated Show resolved Hide resolved
@42atomys
Copy link
Contributor Author

@QPixel Ready to review & launch 🎉

@FedorLap2006
Copy link
Collaborator

You'll also have to update your entire branch with the current state of master. @FedorLap2006 any additional things I didn't catch?

I'll give it a look later this day

@42atomys
Copy link
Contributor Author

42atomys commented Feb 20, 2022

I'll give it a look later this day

Hi @FedorLap2006 , any news ?

@FedorLap2006
Copy link
Collaborator

FedorLap2006 commented Feb 20, 2022

I'll give it a look later this day

Hi @FedorLap2006 , any news ?

Making a review currently. Also please note conflicts.

@42atomys
Copy link
Contributor Author

Amazing conflicts spawn before each review, I'm dead !

@42atomys
Copy link
Contributor Author

Done again !

Copy link
Collaborator

@FedorLap2006 FedorLap2006 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 for the contribution!

Overall PR looks good. Except some moments.

There is an obvious problem with tests. They cover a really small portion of endpoints behaviour (e.g. Update is untested with some parameters being set, unset).
I would recommend also making an example, because this looks like a fairly complex topic for a beginner, and it would give you ability to catch more errors and mistakes in the process.

Please also note my comments.

scheduled_events.go Outdated Show resolved Hide resolved
scheduled_events_test.go Outdated Show resolved Hide resolved
structs.go Outdated Show resolved Hide resolved
structs.go Outdated Show resolved Hide resolved
structs.go Outdated Show resolved Hide resolved
scheduled_events.go Outdated Show resolved Hide resolved
scheduled_events.go Outdated Show resolved Hide resolved
scheduled_events_test.go Outdated Show resolved Hide resolved
scheduled_events_test.go Outdated Show resolved Hide resolved
structs.go Show resolved Hide resolved
@42atomys
Copy link
Contributor Author

@FedorLap2006 I switch to two separated structs and add an example in bonus !

Copy link
Collaborator

@FedorLap2006 FedorLap2006 left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, but has a couple of issues.

examples/scheduled_events/main.go Outdated Show resolved Hide resolved
structs.go Show resolved Hide resolved
nullable_type.go Outdated Show resolved Hide resolved
restapi.go Outdated Show resolved Hide resolved
restapi.go Show resolved Hide resolved
restapi.go Outdated Show resolved Hide resolved
restapi.go Outdated Show resolved Hide resolved
structs.go Outdated Show resolved Hide resolved
@FedorLap2006
Copy link
Collaborator

Also, please note the conflicts.

42atomys and others added 5 commits February 27, 2022 14:55
Co-authored-by: Fedor Lapshin <fe.lap.prog@gmail.com>
Co-authored-by: Fedor Lapshin <fe.lap.prog@gmail.com>
Co-authored-by: Fedor Lapshin <fe.lap.prog@gmail.com>
structs.go Outdated Show resolved Hide resolved
structs.go Outdated Show resolved Hide resolved
@42atomys
Copy link
Contributor Author

@FedorLap2006 all tests run with success

@FedorLap2006
Copy link
Collaborator

@FedorLap2006 all tests run with success

Alright. Yea, everything should be fine, but I still don't completely get why 1 and 1 are there. The purpose is make the test coverage 100% & make function always return len == 0?

discord_test.go Outdated Show resolved Hide resolved
discord_test.go Outdated Show resolved Hide resolved
@42atomys
Copy link
Contributor Author

I still don't completely get why 1 and 1 are there.

Actually with or without 1 and 1, this function on test suite will always return len == 0 because no users can register via the API. Second point, DiscordAPI don't crash with 1 on snowflake -1 or 0 or malformed snowflake will return error

The purpose is make the test coverage 100% & make function always return len == 0

Yes it is, but formulated differently: Up the coverage to 100% on this test that will always return len(users) == 0

42atomys and others added 2 commits February 27, 2022 15:18
Co-authored-by: Fedor Lapshin <fe.lap.prog@gmail.com>
structs.go Outdated Show resolved Hide resolved
restapi.go Outdated Show resolved Hide resolved
discord_test.go Outdated Show resolved Hide resolved
@FedorLap2006
Copy link
Collaborator

Sorry that the review took long, found some additional grammar mistakes.

42atomys and others added 3 commits February 27, 2022 15:51
Co-authored-by: Fedor Lapshin <fe.lap.prog@gmail.com>
Co-authored-by: Fedor Lapshin <fe.lap.prog@gmail.com>
Co-authored-by: Fedor Lapshin <fe.lap.prog@gmail.com>
@42atomys
Copy link
Contributor Author

Sorry that the review took long, found some additional grammar mistakes.

Dont worry, it is appreciated, english is not my natal language, I try to learn it :)

@FedorLap2006
Copy link
Collaborator

Oh good, I don't need to do that myself.

@FedorLap2006
Copy link
Collaborator

I think everything looks good. Thanks for your contribution!

@FedorLap2006 FedorLap2006 merged commit 9448b0e into bwmarrin:master Feb 27, 2022
@42atomys
Copy link
Contributor Author

Thanks for your time ! I will push more soon 😀

A follow is appreciate ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature implementation high priority Issue or PR with high priority of merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants