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 functions to Admin API and there response type #181

Merged
merged 8 commits into from
Feb 24, 2024

Conversation

grvn-ht
Copy link
Contributor

@grvn-ht grvn-ht commented Feb 16, 2024

Hi !!
I'm using your lib to implement a benthos input for synapse and i needed more Admin API so i implemented it in this pull request.
Tell me if it's ok for you to add theses changes.
Have a nice day,
++

Copy link
Member

@tulir tulir left a comment

Choose a reason for hiding this comment

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

In general totally fine with adding new admin API wrappers, but the code quality needs a bit of improvement. Some of the comments apply to more than one place (I didn't want to repeat the same comment multiple times), so please fix all similar points

responses.go Outdated Show resolved Hide resolved
responses.go Outdated Show resolved Hide resolved
synapseadmin/client.go Outdated Show resolved Hide resolved
synapseadmin/roomapi.go Outdated Show resolved Hide resolved
synapseadmin/roomapi.go Outdated Show resolved Hide resolved
synapseadmin/roomapi.go Outdated Show resolved Hide resolved
synapseadmin/userapi.go Outdated Show resolved Hide resolved
synapseadmin/roomapi.go Outdated Show resolved Hide resolved
synapseadmin/roomapi.go Outdated Show resolved Hide resolved
@grvn-ht
Copy link
Contributor Author

grvn-ht commented Feb 16, 2024

Thanks for reviewing, going to fix it

@grvn-ht
Copy link
Contributor Author

grvn-ht commented Feb 20, 2024

Hi @tulir ,
I made requested changes,
Is everything good or are there still things to do?
Have a nice day!

Copy link
Member

@tulir tulir left a comment

Choose a reason for hiding this comment

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

Looking better, but some fields still need better types and the Messages function has a duplicate. Also check the lint errors that GitHub actions reported (or run pre-commit run -a locally)

synapseadmin/client.go Outdated Show resolved Hide resolved
synapseadmin/responses.go Outdated Show resolved Hide resolved
synapseadmin/responses.go Outdated Show resolved Hide resolved
synapseadmin/roomapi.go Outdated Show resolved Hide resolved
synapseadmin/roomapi.go Outdated Show resolved Hide resolved
synapseadmin/roomapi.go Outdated Show resolved Hide resolved
synapseadmin/roomapi.go Outdated Show resolved Hide resolved
synapseadmin/responses.go Outdated Show resolved Hide resolved
synapseadmin/responses.go Outdated Show resolved Hide resolved
@grvn-ht
Copy link
Contributor Author

grvn-ht commented Feb 20, 2024

Thanks for reviewing @tulir !
Everything up to date now
Except for one lint problem in crypto/olm/account.go
fatal error: olm/olm.h: No such file or directory
But I didn't played with this project's part, it should have already been there
Tell me if you see other things to change in my code!

@tulir
Copy link
Member

tulir commented Feb 20, 2024

Ah yeah running pre-commit with -a will run it on all files, which requires libolm-dev to be installed. If you do pre-commit install, it'll check only modified files when running git commit (but that doesn't really help for fixing things after committing).

@grvn-ht
Copy link
Contributor Author

grvn-ht commented Feb 21, 2024

need more changes or all good?

@sumnerevans
Copy link
Member

As Tulir mentioned, the linter must pass.

@grvn-ht
Copy link
Contributor Author

grvn-ht commented Feb 22, 2024

my local config was missing goimport ^^
now it should be good

@sumnerevans sumnerevans requested a review from tulir February 23, 2024 17:44
@tulir tulir merged commit cbd1334 into mautrix:master Feb 24, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants