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

Auditlog cleanup and update #1586

Merged
merged 44 commits into from
Aug 28, 2023

Conversation

Plerx2493
Copy link
Member

@Plerx2493 Plerx2493 commented Jul 7, 2023

Summary

I started to cleanup the GetAuditLogs method on DiscordGuild and updated the AuditLogActionType Enum

Notes

Feel free to give me feedback and point out additional methods i can cleanup in this scope. I thought about rewriting other auditlog related sections but i dont know entirely how it works

Tasks

  • Implement missing entry types
  • Implement parsing of missing types
  • Implement missing AuditLog event

@Plerx2493
Copy link
Member Author

i have not implemented a new type for the new actionTypes because i didnt know if we want to continue with this monster method

@Plerx2493
Copy link
Member Author

The editorconfig is against using var. Should i replace it in all methods i clean up? its used everywhere

@akiraveliara
Copy link
Member

please do, and if you can, also run other format rules like file-scoped namespaces

@Plerx2493
Copy link
Member Author

@akiraveliara should i enforce all editorconfig warnings or only those which only affects files i already have changes in?

@Plerx2493
Copy link
Member Author

There are several naming rules broken but they also make changes in DiscordClient.cs

@akiraveliara
Copy link
Member

only in files you touch for now, so as to avoid huge merge conflicts

@Plerx2493
Copy link
Member Author

ok i got it

@Plerx2493
Copy link
Member Author

image
145...

@Plerx2493
Copy link
Member Author

Now its a huge PR for a minor change ^^

@akiraveliara
Copy link
Member

you can always make the PR extend to more audit log functionality if you want (:

@akiraveliara
Copy link
Member

but seriously, that's fine. it'll be a little annoying when it comes to reverting commits, but eh

@Plerx2493
Copy link
Member Author

My plan is to implement the missing auditlogtypes. I will see what I can do after that

@akiraveliara
Copy link
Member

may close #1569

Copy link
Member

@InFTord InFTord left a comment

Choose a reason for hiding this comment

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

more documentation required, but other than that LGTM

@Plerx2493
Copy link
Member Author

may close #1569

I haven't had a chance to check it yet, but I can look at it later.

@Plerx2493
Copy link
Member Author

more documentation required, but other than that LGTM

xml docs on the new methods or anything else? I have those in mind

@InFTord
Copy link
Member

InFTord commented Aug 11, 2023

yes, xmldocs

Copy link
Member

@akiraveliara akiraveliara left a comment

Choose a reason for hiding this comment

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

lgtm, will wait for preliminary test results before merging - if you want to split the types off into their own files, feel free

@Plerx2493
Copy link
Member Author

lgtm, will wait for preliminary test results before merging - if you want to split the types off into their own files, feel free

currently working on it

@Plerx2493
Copy link
Member Author

Should be ready now

Copy link
Member

@InFTord InFTord left a comment

Choose a reason for hiding this comment

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

LGTM

@akiraveliara akiraveliara mentioned this pull request Aug 13, 2023
21 tasks
@Plerx2493
Copy link
Member Author

Plerx2493 commented Aug 14, 2023

Missing change keys:

DiscordAuditLogAutoModerationRuleEntry:

  • $add_keyword_filter
  • $remove_keyword_filter

DiscordAuditLogIntegrationEntry:

  • type
  • name

DiscordAuditLogThreadEventEntry:

  • flags

@akiraveliara
Copy link
Member

once the change keys are added we can merge this as far as i'm concerned

@Plerx2493
Copy link
Member Author

should be ready to merge

@akiraveliara akiraveliara merged commit 5bc3201 into DSharpPlus:master Aug 28, 2023
1 check passed
@Plerx2493 Plerx2493 mentioned this pull request Aug 29, 2023
@Plerx2493 Plerx2493 deleted the dev/auditlogUpdate branch October 5, 2023 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants