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: ThreadMember flags not serialized #1335

Merged
merged 1 commit into from Mar 9, 2023

Conversation

armsnyder
Copy link
Contributor

The Flags field was being incorrectly de/serialized as "Flags".

ThreadMember spec: https://discord.com/developers/docs/resources/channel#thread-member-object

I caught this with the musttag linter, so I went ahead and added it to the linter config.

@FedorLap2006 FedorLap2006 added the fix Pull requests and issues related to bug fixes and structural inconsistencies label Mar 9, 2023
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.

Thanks for catching yet another oversight.

One thing to note though. I'm not entirely sure about the usage of musttag linter.
At least currently, It does not fullfil our needs. As an example:

  • We tend to have a lot of nested structures, but after some local testing, it doesn't seem to catch any changes in them.
  • Most of the structures are used in REST API functions, but the linter also doesn't seem catch any change in them.

In any case, it's going to be a good idea to separate the linter change into a different PR, to make things easier.

@FedorLap2006 FedorLap2006 modified the milestones: v0.27.1, v0.28.0 Mar 9, 2023
@armsnyder
Copy link
Contributor Author

armsnyder commented Mar 9, 2023

@FedorLap2006 It sounds like you are concerned about false negatives, which is the case with most linters. They safeguard against some but not all bugs. In this case the musttag linter did catch the Flags bug.

I removed the commit from this PR and opened #1345

@FedorLap2006
Copy link
Collaborator

Great! Since I'm still writing up the release notes for the v0.27.1, we can probably merge this as well.

@FedorLap2006 FedorLap2006 merged commit 1115a47 into bwmarrin:master Mar 9, 2023
@armsnyder armsnyder deleted the thread-member branch March 9, 2023 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Pull requests and issues related to bug fixes and structural inconsistencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants